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

Status: RESOLVED FIXED

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

Description

[Julian Seward [:jseward]]

(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.

Comment 1

[Julian Seward [:jseward]]

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

Comment 2

[Julian Seward [:jseward]]

(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?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

oh, hmm, GetPrimaryPresState() can be slow too.
Not quite sure what the best approach is.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: savestate.diff

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76cb13fa1a00
create element state objects only if needed, r=ehsan

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Hopefully that helps :)

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/76cb13fa1a00

Comment 9

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/76cb13fa1a003ef219cdebf7eaf82c7aeca3b56e
Bug 1386582, create element state objects only if needed, r=ehsan