perf drain interaction with assigning to InnerHTML while a mutation observer exists
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox91 | --- | fixed |
People
(Reporter: jhudson, Assigned: emilio)
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0
Steps to reproduce:
Ref: https://github.com/uBlockOrigin/uBlock-issues/issues/1620
A web application I was developing got some nuts performance issues while uBlock origin was active on the page despite blocking nothing at the time. My bisection was assigning to .innerHTML inside a react component would hang for 13 seconds while the same would not hang on an isolated page. But using animation frames to hide it from react's DOM differ didn't work.
The uBO team managed to convince themselves the issue is in Firefox since they don't do anything interesting when assigning to .innerHTML.
I have a 5MB zip file reproducing the issue but it's unsuitable for public upload.
|
||
Comment 2•4 years ago
|
||
The Bugbug bot thinks this bug should belong to the 'Core::Layout' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.
|
Assignee | |
Comment 3•4 years ago
|
||
Can you take a profile of this with https://profiler.firefox.com, with the "platform" preset?
|
|
Assignee | |
Comment 4•4 years ago
|
||
Or, feel free to send me the repro and I can try to look. Though something publicly accessible would be ideal so other devs can also take a look.
Are these set right?
Bad performance profiler (uBlock origin ON): https://share.firefox.dev/3pB1kLj
Good performance profiler (uBlock origin OFF): https://share.firefox.dev/2RDrIaG
|
|
Assignee | |
Comment 6•4 years ago
|
||
Reporter sent a link to a place where I can repro (though for some reason on some configurations I can't, I could repro the massive perf drop with uBlock enabled on my personal profile). Will post details in a private comment after this since the reporter didn't want the URL to be public.
Thanks so much jhudson.
|
|
Assignee | |
Comment 8•4 years ago
|
||
The profiles in comment 5 didn't have any native callstack, for some reason, but I took one locally: https://share.firefox.dev/3iGq9UC
All the time seems spent in CompareDocumentPosition in nsContentList::ContentAppended, on the bad case. That's probably this PositionIsBefore call, which is meant to be an optimization, hah :)
So this is not about mutationobserver, but more about uBlock doing getElementsByClassName or such thing (which are live collections and require extra book-keeping).
|
||
Comment 9•4 years ago
•
|
||
more about uBlock doing getElementsByClassName or such thing (which are live collections and require extra book-keeping)
Would the issue go away if I used something like querySelectorAll('[class]')? I went with getElementsByClassName() because I thought it would be more performant as it's purpose is more specific.
Oops, sorry, I had assumed uBO was actually calling getElementsByClassName(), but I see no such call in the code.
|
|
Assignee | |
Comment 10•4 years ago
|
||
(In reply to rhill@raymondhill.net from comment #9)
Oops, sorry, I had assumed uBO was actually calling
getElementsByClassName(), but I see no such call in the code.
I can track down which specific call causes this in a bit :)
|
|
||
Comment 11•4 years ago
•
|
||
I do use getElementsByTagName() though, which I believe returns a live collection -- but the result of the call is used locally in a function call, so the live collection should no longer exist when exiting the function.
|
|
Assignee | |
Comment 12•4 years ago
|
||
(But anyways we should probably remove that optimization, or make it somewhat smarter... Olli, does comment 8 ring a bell about something that might be on file? What do you think about removing that optimization? Other ideas?)
|
|
Assignee | |
Comment 13•4 years ago
|
||
(In reply to rhill@raymondhill.net from comment #11)
I do use
getElementsByTagName()though, which I believe returns a live collection -- but the result of the call is used locally in a function call.
Yeah, that's right... Probably it's kept alive by the JS engine until the next GC. Anyhow will look closer to see which particular collection is triggering this.
|
|
Assignee | |
Comment 14•4 years ago
|
||
Ok, so here's the interesting stack. It's not a getElementsBy* call what's problematic, .children, via .childElementCount, via this stack:
0 onDOMChanged(addedNodes = "https://public.cedaronmedical.com/message.html?archiveMessageId=19,https://public.cedaronmedical.com/message.html?archiveMessageId=18,[object HTMLPreElement],[object HTMLDivElement],[object HTMLDivElement]", "true") ["moz-extension://6ebeefcf-544b-4f7b-bef9-b75535c06677/js/contentscript.js":938:21]
this = [object Object]
1 safeObserverHandler() ["moz-extension://6ebeefcf-544b-4f7b-bef9-b75535c06677/js/contentscript.js":363:27]
this = [object Object]
2 onRAF() ["moz-extension://6ebeefcf-544b-4f7b-bef9-b75535c06677/js/contentscript.js":234:13]
this = [object Object]
3 start/this.fid<("14665.94") ["moz-extension://6ebeefcf-544b-4f7b-bef9-b75535c06677/js/contentscript.js":203:63]
So here's the catch:
- uBlock calls
childElementCounton a (for now empty)<pre>element. - Then that
<pre>element is populated with ~80k children. For each those appends, we go throughnsContentList::ContentAppended, usually hitting the fast path here, but not when we have just appended a newline, and that's enough to destroy perf on this test-case...
I have two tweaks that should fix this.
|
|
Assignee | |
Comment 15•4 years ago
|
||
Two tweaks:
-
Remove useless count > 0 check. We're in ContentAppended and have
checked for NAC before, so this condition is just useless. This
should be mostly irrelevant. -
If the container is our root node, then we're necessarily appending,
so no need to check CompareDocumentPosition. This improves
performance in cases like the test-case above, with lots of children
and some text nodes in between.
|
|
Assignee | |
Comment 16•4 years ago
|
||
If the element ever gets children, then the live collection would slow
things down.
It's unclear if the collection here is worth it at all, fwiw.
I guess it could if someone writes code looping around
childElementCount, or calls it repeatedly with a lot of children...
But maybe we should just implement it the dumb way? FWIW, WebKit and
Blink both do the equivalent of just walking over the child nodes
counting elements... Perhaps follow-up bug if you think it's worth doing
the same?
Depends on D118032
|
||
Comment 17•4 years ago
|
||
|
|
||
Comment 18•4 years ago
|
||
In uBO, instead of using:
if ( node.childElementCount === 0 ) { continue; }
Would it help (and is it worth) to instead use:
if ( node.firstChild === null ) { continue; }
To improve performance until the above fix becomes available in Firefox stable?
|
|
Assignee | |
Comment 19•4 years ago
|
||
Yeah, that'd help. The strictly equivalent API is firstElementChild, if you care about the distinction between elements and text nodes. That should be strictly faster since you don't need to count over all children.
|
||
Comment 20•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/b1ee3d0e1b0c
https://hg.mozilla.org/mozilla-central/rev/e5536a556a05
|
Reporter | |
Comment 21•4 years ago
|
||
When can we take the URL down?
|
|
Assignee | |
Comment 22•4 years ago
|
||
You can now. It should be fixed on latest nightly and the change should be in FF91
|
|
Assignee | |
Comment 23•4 years ago
|
||
(and maybe uBo has been tweaked to avoid this footgun, not sure)
|
||
Comment 24•2 years ago
|
||
Clearing this needinfo (as part of going through my old needinfos :) ).
Description
•