Closed
Bug 1386582
Opened 6 years ago
Closed 6 years ago
Very short-term heap allocation in mozilla::dom::HTMLInputElement::SaveState()
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jseward, Assigned: smaug)
Details
Attachments
(1 file)
4.31 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(this is SaveState() at dom/html/HTMLInputElement.cpp) Profiling Gecko running 20 out of 420 iterations of http://browserbench.org/Speedometer, I see 5661 heap allocations arising from SaveState(), the average lifetime of which is only 127 instructions. Also, there is only one such allocation ever alive at once. What I interpret this to mean is that we are creating |inputState| on the heap and that stays alive only for the duration of this function. Would it be possible to change the type of |inputState| from |RefPtr<HTMLInputElementState>|, so it can be stack allocated? Or something similar? One bit I don't understand is that my numbers suggest that the objects pointed to by |inputState| don't stay alive after this function returns. But I also see this (line 6457): if (inputState) { nsPresState* state = GetPrimaryPresState(); if (state) { state->SetStateProperty(inputState); } } so it looks as if we're copying |inputState| off somewhere else? I am a bit confused.
Reporter | ||
Comment 1•6 years ago
|
||
Here's the relevant DHAT record. BTW, it looks from this that there is a 6-byte alignment hole at the end of class HTMLInputElementState. Not much we can do about that, I think. -------------------- 52 of 1000 -------------------- max-live: 48 in 1 blocks tot-alloc: 271,728 in 5,661 blocks (avg size 48.00) deaths: 5,661, at avg age 127 (0.00% of prog lifetime) acc-ratios: 0.70 rd, 1.37 wr (192,474 b-read, 373,626 b-written) at 0x4C2BF9D: malloc (/home/sewardj/VgTRUNK/progress/coregrind/m_replacemalloc/vg_replace_malloc.c:299) by 0x4062E8: moz_xmalloc (memory/mozalloc/mozalloc.cpp:83) by 0x122DC11C: operator new (gcc-O2-nondebug-jemalloc/dist/include/mozilla/mozalloc.h:194) by 0x122DC11C: mozilla::dom::HTMLInputElement::SaveState() (dom/html/HTMLInputElement.cpp:6417) by 0x12340F99: nsGenericHTMLFormElement::UnbindFromTree(bool, bool) (dom/html/nsGenericHTMLElement.cpp:1910) by 0x122D2341: mozilla::dom::HTMLInputElement::UnbindFromTree(bool, bool) (dom/html/HTMLInputElement.cpp:4888) by 0x11862614: mozilla::dom::Element::UnbindFromTree(bool, bool) (dom/base/Element.cpp:1994) by 0x12340E21: nsGenericHTMLElement::UnbindFromTree(bool, bool) (dom/html/nsGenericHTMLElement.cpp:538) Aggregated access counts by offset: [ 0] 11322 11322 11322 11322 11322 11322 11322 11322 22644 22644 22644 22644 22644 22644 22644 22644 [ 16] 11322 11322 11322 11322 11322 11322 11322 11322 5661 5661 5661 5661 11322 11322 5661 5661 [ 32] 16983 16983 16983 16983 16983 16983 16983 16983 5661 5661 0 0 0 0 0 0
Reporter | ||
Comment 2•6 years ago
|
||
(In reply to Julian Seward [:jseward] from comment #0) > Would it be possible to change the type of |inputState| from > |RefPtr<HTMLInputElementState>|, so it can be stack allocated? Duh. I mean to write: Would it be possible to change the type of |inputState| from |RefPtr<HTMLInputElementState>| to |Maybe<HTMLInputElementState>|, so it can be stack allocated?
Assignee | ||
Comment 3•6 years ago
|
||
PresState doesn't copy, but takes a reference. So, is it possible that GetPrimaryPresState(); returns null and the method just does unneeded work, since the input state isn't stored anywhere.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 4•6 years ago
|
||
oh, hmm, GetPrimaryPresState() can be slow too. Not quite sure what the best approach is.
Assignee: bugs → nobody
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 5•6 years ago
|
||
remote: View your change here: remote: https://hg.mozilla.org/try/rev/4a78aea5debe20abaec8afe21c5b45b9f1e78b66 remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a78aea5debe20abaec8afe21c5b45b9f1e78b66
Attachment #8892926 -
Flags: review?(ehsan)
Updated•6 years ago
|
Attachment #8892926 -
Flags: review?(ehsan) → review+
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/76cb13fa1a00 create element state objects only if needed, r=ehsan
Assignee | ||
Comment 7•6 years ago
|
||
Hopefully that helps :)
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/76cb13fa1a00
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 9•6 years ago
|
||
https://hg.mozilla.org/projects/date/rev/76cb13fa1a003ef219cdebf7eaf82c7aeca3b56e Bug 1386582, create element state objects only if needed, r=ehsan
You need to log in
before you can comment on or make changes to this bug.
Description
•