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

RESOLVED FIXED in Firefox 57

Status

()

Core
HTML: Parser
P2
normal
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: Ehsan, Assigned: hsivonen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
See http://bit.ly/2t1bvxB

Looks like bug 1347737 has already improved things here a bit.  Is there room for any further improvement?
(Assignee)

Comment 1

10 months ago
(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.
(Reporter)

Comment 2

10 months ago
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
(Assignee)

Comment 4

9 months ago
(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)
(Reporter)

Comment 5

9 months ago
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)
(Reporter)

Comment 6

9 months ago
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
(Assignee)

Comment 7

9 months ago
(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.
(Assignee)

Comment 8

9 months ago
(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.
(Assignee)

Comment 9

9 months ago
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.
(Assignee)

Comment 10

9 months ago
(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)

Updated

9 months ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Reporter)

Comment 11

9 months ago
Thanks Henri, the proposal in comment 10 sounds great!
Comment hidden (mozreview-request)
(Assignee)

Comment 13

8 months ago
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.
(Assignee)

Updated

8 months ago
Attachment #8897342 - Flags: review?(ehsan)
(Reporter)

Comment 14

8 months ago
mozreview-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

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 hidden (mozreview-request)
(Assignee)

Comment 16

8 months ago
mozreview-review-reply
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.

Comment 17

8 months ago
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
(Assignee)

Comment 18

8 months ago
(In reply to Henri Sivonen (:hsivonen) from comment #16)
> I'll file a follow-up.

Thanks for the r+. Filed bug 1390504.
(Assignee)

Comment 19

8 months ago
(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.
(Assignee)

Comment 20

8 months ago
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)
Comment hidden (mozreview-request)

Comment 23

8 months ago
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
(Assignee)

Comment 24

8 months ago
Relanded with trivial fix. (Added "explicit".)
Flags: needinfo?(hsivonen)

Comment 25

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8c6d10611354
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1398401
You need to log in before you can comment on or make changes to this bug.