Closed Bug 1715841 Opened 3 years ago Closed 3 years ago

perf drain interaction with assigning to InnerHTML while a mutation observer exists

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
Firefox 89
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
91 Branch
Tracking Flags:
Tracking Status
firefox91 --- fixed

People

(Reporter: jhudson, Assigned: emilio)

Details

Attachments

(2 files)

Bug 1715841 - Make nsContentList::ContentAppended faster in some cases. r=smaug,peterv
48 bytes, text/x-phabricator-request
Details | Review
Bug 1715841 - Don't bother creating a live collection when childElementCount is called on an empty element. r=smaug,peterv
48 bytes, text/x-phabricator-request
Details | Review

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.

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.

Component: Untriaged → Layout
Product: Firefox → Core

Can you take a profile of this with https://profiler.firefox.com, with the "platform" preset?

Component: Layout → DOM: Core & HTML

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

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true

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).

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.

(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 :)

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.

(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?)

Flags: needinfo?(bugs)

(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.

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 childElementCount on a (for now empty) <pre> element.
  • Then that <pre> element is populated with ~80k children. For each those appends, we go through nsContentList::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: nobody → emilio

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.

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

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1ee3d0e1b0c
Make nsContentList::ContentAppended faster in some cases. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e5536a556a05
Don't bother creating a live collection when childElementCount is called on an empty element. r=smaug

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?

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.

https://hg.mozilla.org/mozilla-central/rev/b1ee3d0e1b0c
https://hg.mozilla.org/mozilla-central/rev/e5536a556a05

Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox91: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch

When can we take the URL down?

You can now. It should be fixed on latest nightly and the change should be in FF91

(and maybe uBo has been tweaked to avoid this footgun, not sure)

Clearing this needinfo (as part of going through my old needinfos :) ).

Flags: needinfo?(smaug)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: