Closed Bug 1386582 Opened 3 years ago Closed 3 years ago

Very short-term heap allocation in mozilla::dom::HTMLInputElement::SaveState()


(Core :: DOM: Core & HTML, enhancement)

Not set



Tracking Status
firefox57 --- fixed


(Reporter: jseward, Assigned: smaug)



(1 file)

(this is SaveState() at dom/html/HTMLInputElement.cpp)

Profiling Gecko running 20 out of 420 iterations of, 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

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) {

so it looks as if we're copying |inputState| off somewhere else?  I am a bit
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
(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?
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: nobody → bugs
oh, hmm, GetPrimaryPresState() can be slow too.
Not quite sure what the best approach is.
Assignee: bugs → nobody
Assignee: nobody → bugs
Attached patch savestate.diffSplinter Review
remote: View your change here:
remote: Follow the progress of your build on Treeherder:
Attachment #8892926 - Flags: review?(ehsan)
Attachment #8892926 - Flags: review?(ehsan) → review+
Pushed by
create element state objects only if needed, r=ehsan
Hopefully that helps :)
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.