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)
Core
DOM: HTML Parser
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?
Assignee | ||
Comment 1•7 years 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•7 years ago
|
||
William, any chance you can have a look at this one?
Flags: needinfo?(wchen)
Comment 3•7 years ago
|
||
The other day William told me he's working on this.
Assignee: nobody → wchen
Flags: needinfo?(wchen)
Updated•7 years ago
|
Assignee: wchen → nobody
Priority: -- → P2
Assignee | ||
Comment 4•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Reporter | ||
Comment 11•7 years ago
|
||
Thanks Henri, the proposal in comment 10 sounds great!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years 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•7 years ago
|
Attachment #8897342 -
Flags: review?(ehsan)
Reporter | ||
Comment 14•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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.
Comment 21•7 years ago
|
||
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•7 years 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•7 years ago
|
||
Relanded with trivial fix. (Added "explicit".)
Flags: needinfo?(hsivonen)
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8c6d10611354
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•