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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jseward, Assigned: smaug)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
(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

2 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

2 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

2 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

2 years ago
Assignee: nobody → bugs
(Assignee)

Comment 4

2 years ago
oh, hmm, GetPrimaryPresState() can be slow too.
Not quite sure what the best approach is.
Assignee: bugs → nobody
(Assignee)

Updated

2 years ago
Assignee: nobody → bugs
(Assignee)

Comment 5

2 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

2 years ago
Attachment #8892926 - Flags: review?(ehsan) → review+

Comment 6

2 years ago
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

2 years ago
Hopefully that helps :)

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76cb13fa1a00
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.