Closed Bug 1375701 Opened 7 years ago Closed 7 years ago

Consider using some kind of auto allocation to avoid free() cost in nsHTML5String::Release

Categories

(Core :: DOM: HTML Parser, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

See http://bit.ly/2t1bvxB

Looks like bug 1347737 has already improved things here a bit.  Is there room for any further improvement?
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #0)
> See http://bit.ly/2t1bvxB

That's weird. The way it's meant to work is that nsHtml5String allocates an nsStringBuffer that qualifies for adoption into the DOM's attribute storage (https://searchfox.org/mozilla-central/source/dom/base/nsAttrValue.cpp#1937) so that it doesn't need to be freed during the parse.

If that's not happening, the first step would be figuring out why it isn't happening.
William, any chance you can have a look at this one?
Flags: needinfo?(wchen)
The other day William told me he's working on this.
Assignee: nobody → wchen
Flags: needinfo?(wchen)
Assignee: wchen → nobody
Priority: -- → P2
(In reply to Henri Sivonen (:hsivonen) from comment #1)
> That's weird. The way it's meant to work is that nsHtml5String allocates an
> nsStringBuffer that qualifies for adoption into the DOM's attribute storage
> (https://searchfox.org/mozilla-central/source/dom/base/nsAttrValue.cpp#1937)
> so that it doesn't need to be freed during the parse.
> 
> If that's not happening, the first step would be figuring out why it isn't
> happening.

A superficial investigation with gdb indicates that the nsStringBuffer gets indeed adopted into nsAttrValue, in which case nsHtml5String::Release() should not end up calling free().

Ehsan, do you have a test case that shows nsHtml5String::Release() calling free() too much?
Flags: needinfo?(ehsan)
Thanks for looking into this.

Comment 0 was from a full run of Speedometer.  I typically profile this by going to http://speedometer2.benj.me/, press Start Test, wait for about 100 or so of the tests to run (because otherwise the profiles becomes too big for Gecko Profiler to handle) and will then capture a profile.  This profile however is from a couple of months ago so I don't remember the exact steps I used to record it, but this is how I record almost all of the Speedometer profiles for all of the bugs I file about it.

A cursory look at the code suggests that we are taking this branch <https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/parser/html/nsHtml5Tokenizer.cpp#344> which would happen if newAttributesEachTime is false, which it seems can happen if nsHtml5TreeBuilder is constructed using this constructor (which sets mBuilder to nullptr) <https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/parser/html/nsHtml5TreeBuilderCppSupplement.h#38>.  Not sure if any of this is on to something...
Flags: needinfo?(ehsan)
FWIW I just captured this profile from a run of ~150 subtests of Speedometer on trunk, with a profiler interval of 0.1ms and a buffer size of 450MB: https://perfht.ml/2vNgCmV
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away Aug 7-8) from comment #5)
> Thanks for looking into this.
> 
> Comment 0 was from a full run of Speedometer.  I typically profile this by
> going to http://speedometer2.benj.me/,

Thanks. I see the problem with the first 150 subtests there.

> A cursory look at the code suggests that we are taking this branch
> <https://searchfox.org/mozilla-central/rev/
> bd39b6170f04afeefc751a23bb04e18bbd10352b/parser/html/nsHtml5Tokenizer.
> cpp#344> which would happen if newAttributesEachTime is false

This means that (on Speedometer v2), the problem occurs with innerHTML (as opposed to parsing HTML from network).

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away Aug 7-8) from comment #6)
> FWIW I just captured this profile from a run of ~150 subtests of Speedometer
> on trunk, with a profiler interval of 0.1ms and a buffer size of 450MB:
> https://perfht.ml/2vNgCmV

I get 403 forbidden with this URL when perf-html.io tries to load the profile.
(In reply to Henri Sivonen (:hsivonen) from comment #1)
> The way it's meant to work is that nsHtml5String allocates an
> nsStringBuffer that qualifies for adoption into the DOM's attribute storage
> (https://searchfox.org/mozilla-central/source/dom/base/nsAttrValue.cpp#1937)
> so that it doesn't need to be freed during the parse.

The adoption into the DOM's attribute storage seems to be working as designed for every attribute values that occurs in innerHTML in the first 150 subtests of Speedometer v2 and reaches the above-referenced code.

Up next: Figuring out if there are attribute values that don't reach the DOM's attribute storage at all.
Early on, there's a <ul> whose innerHTML is set to "<li data-id=\"null\" class=\"\"><div class=\"view\"><input class=\"toggle\" type=\"checkbox\" ><label>Something to do 0</label><button class=\"destroy\"></button></div></li>" and all the attribute values get freed.

Worth noting, though, that this doesn't repeat a lot early on, so it's not like every innerHTML assignment frees the attribute values.
(In reply to Henri Sivonen (:hsivonen) from comment #4)
> (In reply to Henri Sivonen (:hsivonen) from comment #1)
> > That's weird. The way it's meant to work is that nsHtml5String allocates an
> > nsStringBuffer that qualifies for adoption into the DOM's attribute storage
> > (https://searchfox.org/mozilla-central/source/dom/base/nsAttrValue.cpp#1937)
> > so that it doesn't need to be freed during the parse.
> > 
> > If that's not happening, the first step would be figuring out why it isn't
> > happening.
> 
> A superficial investigation with gdb indicates that the nsStringBuffer gets
> indeed adopted into nsAttrValue, in which case nsHtml5String::Release()
> should not end up calling free().

The reason why I didn't see the problem even when deliberately testing attributes that I suspected got atomized was that I only tested with values that didn't have atoms already and the dynamic atoms adopted the nsStringBuffers.

However, since the same values repeat in Speedometer, the subsequent copies of the already-seen atomized attribute values get freed, since the atom already exists and doesn't need another backing buffer.

Instead of going with the solution proposed in the bug description, my inclination would be to try to make nsHtml5String able to hold either an nsStringBuffer or an nsIAtom and making the parser look up the Atom at least is the most perf-sensitive case, which is the class attribute having a non-empty value that has no whitespace.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Thanks Henri, the proposal in comment 10 sounds great!
The patch intentionally addresses only the innerHTML/DOMParser/createContextualFragment case and not the network / document.write() case.

Most likely the latter is worth doing, too, but it would involve the parser thread obtaining a mutex that it's currently not obtaining (the atom table mutex), so I want to put the change in a different changeset in a different nightly for bisect attribution.
Attachment #8897342 - Flags: review?(ehsan)
Comment on attachment 8897342 [details]
Bug 1375701 - Atomize class attribute value in the parser in the innerHTML case.

https://reviewboard.mozilla.org/r/168648/#review174012

Very nice, thanks a lot!

::: dom/base/Element.cpp:2424
(Diff revision 1)
> +  nsIDocument* document = GetComposedDoc();
> +  mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, false);
> +
> +  // In principle, BeforeSetAttr should be called here if a node type
> +  // existed that wanted to do something special for class, but there
> +  // is no such node type, so calling SetMayHaveClass() directly.

Hmm, I suppose if in the future a node type starts to want to do something like that, then going through this path would result in a different side effect and possibly an observable bug.  That being said, I think avoiding the virtual call for now is the correct choice, I can't think of anything better  to do here.  Perhaps it may be a good idea to add a comment about this to Element::BeforeSetAttr() around <https://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/base/Element.cpp#2712> as well?

::: parser/html/nsHtml5Portability.cpp:23
(Diff revision 1)
>  
> +static bool
> +ContainsWhiteSpace(mozilla::Span<char16_t> aSpan)
> +{
> +  for (char16_t c : aSpan) {
> +    if (nsContentUtils::IsHTMLWhitespace(c)) {

Do you think we should consider inlining this function BTW?  I see non-inlined code like this show up in profiles quite frequently...

(Could be a follow-up of course.)
Attachment #8897342 - Flags: review?(ehsan) → review+
Comment on attachment 8897342 [details]
Bug 1375701 - Atomize class attribute value in the parser in the innerHTML case.

https://reviewboard.mozilla.org/r/168648/#review174012

> Do you think we should consider inlining this function BTW?  I see non-inlined code like this show up in profiles quite frequently...
> 
> (Could be a follow-up of course.)

I'll file a follow-up.
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fabf345eec6e
Atomize class attribute value in the parser in the innerHTML case. r=Ehsan
(In reply to Henri Sivonen (:hsivonen) from comment #16)
> I'll file a follow-up.

Thanks for the r+. Filed bug 1390504.
(In reply to Henri Sivonen (:hsivonen) from comment #13)
> The patch intentionally addresses only the
> innerHTML/DOMParser/createContextualFragment case and not the network /
> document.write() case.
> 
> Most likely the latter is worth doing, too, but it would involve the parser
> thread obtaining a mutex that it's currently not obtaining (the atom table
> mutex), so I want to put the change in a different changeset in a different
> nightly for bisect attribution.

Filed bug 1390506.
https://hg.mozilla.org/projects/htmlparser/rev/29b1d27f3ff14996bb58664e2e597f4823261920
Mozilla bug 1375701 - Atomize class attribute value in the parser in the innerHTML case. r=Ehsan.
Backed out for bustage at parser/html/nsHtml5String.h:143:3: bad implicit conversion constructor for 'nsHtml5String':

https://hg.mozilla.org/integration/autoland/rev/76eecfca4bc68248176e48a63efd147e16ec135d

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=76eecfca4bc68248176e48a63efd147e16ec135d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123268116&repo=autoland

[task 2017-08-15T14:20:40.474479Z] 14:20:40     INFO -  In file included from /home/worker/workspace/build/src/parser/html/nsHtml5AttributeName.cpp:32:
[task 2017-08-15T14:20:40.474552Z] 14:20:40     INFO -  /home/worker/workspace/build/src/parser/html/nsHtml5String.h:143:3: error: bad implicit conversion constructor for 'nsHtml5String'
[task 2017-08-15T14:20:40.474596Z] 14:20:40     INFO -    nsHtml5String(uintptr_t aBits) : mBits(aBits) {};
[task 2017-08-15T14:20:40.474619Z] 14:20:40     INFO -    ^
[task 2017-08-15T14:20:40.474678Z] 14:20:40     INFO -  /home/worker/workspace/build/src/parser/html/nsHtml5String.h:143:3: note: consider adding the explicit keyword to the constructor
[task 2017-08-15T14:20:40.474708Z] 14:20:40     INFO -  1 error generated.
Flags: needinfo?(hsivonen)
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c6d10611354
Atomize class attribute value in the parser in the innerHTML case. r=Ehsan
Relanded with trivial fix. (Added "explicit".)
Flags: needinfo?(hsivonen)
https://hg.mozilla.org/mozilla-central/rev/8c6d10611354
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1398401
Depends on: 1405568
You need to log in before you can comment on or make changes to this bug.