Closed Bug 1499170 Opened 6 years ago Closed 5 years ago

Panorama appliance is exceedingly slow in Firefox (selector-matching in quirks-mode document)

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Performance Impact medium
Tracking Status
firefox65 --- fixed

People

(Reporter: justdave, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: perf:responsiveness)

Attachments

(1 file)

We have Palo Alto firewalls at Mozilla, and the Panorama web interface for managing them is exceedingly slow in Firefox lately (Developer Edition 63.0b14 on Mac with a clean profile), while it's quite speedy in Safari.

It's a Javascript-heavy application, and uses layered divs for dialog boxes.  Opening a dialog box for editing a virtual router takes 15 to 30 seconds in Firefox where it takes about 3 seconds in Safari.
Peformance profile while opening a virtual router dialog:
https://perfht.ml/2OYGGEU
(In reply to Dave Miller [:justdave] (justdave@bugzilla.org) from comment #1)
> Peformance profile while opening a virtual router dialog:
> https://perfht.ml/2OYGGEU

I don't have any experiences in ExtJS but it looks like it tries to add something (box? line item in a table?) into the DOM tree and read back its computed style one at the time, in |doLayout| loop. But still according to the call tree most of the time spent is in JS, so that's probably not the cause.
Summary: Panorama appliance is exceedingly slow in Firefox (but fast in SafarI) → Panorama appliance is exceedingly slow in Firefox (but fast in SafarI; ExtJS spent 30+ sec in doLayout)
60% of the profile is spent in stylo (noticeable if you invert the call stacks), esp. style::gecko_string_cache::WeakAtom::eq_ignore_ascii_case, from getComputedStyle, which looks like it's the string cache? I'm assuming either Emilio or Henri know how to move forward with this, though I wonder if there's an easy way for them to replicate this - Dave?
Component: General → DOM: CSS Object Model
Flags: needinfo?(justdave)
Flags: needinfo?(hsivonen)
Flags: needinfo?(emilio)
Product: Firefox → Core
Whiteboard: [qf]
Having a URL would be appreciated. We're resolving the style of an element in a display: none subtree over and over.

We're supposed to have a cache for that, but DOM mutations thrash that cache.

Of course WebKit is fast, but it's incorrect: https://bugs.webkit.org/show_bug.cgi?id=186882
Yeah, without an URL this is hardly actionable, any chance I could get one? I'm curious about the selectors that are going on there.

Also, the page is quirks-mode it seems? That's why we need to do the case-insensitive comparison.

As soon as there's an URL I'm happy to take a look, please ni? me, otherwise this is pretty unactionable.

There's stuff we should optimize there, but I'm not sure at what level... It may be that the cache is not working correctly, or that the selectors are really long and such and we should add a cache for that / lowercase them beforehand...

Moving to the more general CSS component because this codepath also affects normal style flushes. We just don't seem to get a lot of those.
Component: DOM: CSS Object Model → CSS Parsing and Computation
Flags: needinfo?(hsivonen)
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> Also, the page is quirks-mode it seems? That's why we need to do the
> case-insensitive comparison.

Avoiding the case-insensitive comparison would probably mean storing lowercased versions of all attribute name and values in the DOM, I guess?  Or depending on what the page is doing it might be enough to just do it for id and class.
Right... It's tricky, for attributes at least.

Classes and IDs in WebKit and Blink are stored lowercased and atomized in quirks mode documents, looking at the source:

  https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/element_data.h?l=57&rcl=ab0e67a24d2bfab83c2580c853600314bd2d7a47

(The id is stored out of band, if you grep for idForStyleResolution you'll find it)

We need to atomize the thing already, so maybe it's not a huge deal.

But again, a link / repro would be extremely helpful.
It's an internal appliance at Mozilla, it's not on the public web.  If there's a Mozilla employee working on this who has the company VPN set up on their computer, I can probably get you read-only access to it for purpose of debugging this.
Flags: needinfo?(justdave)
Unfortunately, Palo Alto firewalls don't offer an online live demo to play around (couldn't find it by causally searching).
I happen to be a moco employee... I can figure out how to set up the VPN sometime this week :)
Flags: needinfo?(emilio)
I have a VPN setup now (my moco ldap thing is ealvarez@). Any chance I can see the website?
Flags: needinfo?(emilio) → needinfo?(justdave)
Whiteboard: [qf] → [qf:p2]
Whiteboard: [qf:p2] → [qf:p2:responsiveness]
Thanks! I'll take a look tomorrow :)
Flags: needinfo?(justdave) → needinfo?(emilio)
Priority: -- → P2
> Sorry, you do not have permission to access panorama.mozilla.net. Please contact eus@mozilla.com if you should have access. 

:/
Flags: needinfo?(emilio) → needinfo?(justdave)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)
> > Sorry, you do not have permission to access panorama.mozilla.net.

OK, should be fixed now.  One more moving part we missed. :-)
Flags: needinfo?(justdave)
Alright, will try again! :)
Flags: needinfo?(emilio)
And thus massively speed up ascii-case-insensitive atom comparisons when both
atoms are lowercase (which is the common case by far).

This removes almost all the slow selector-matching in this page, and it seems
an easier fix than storing the lowercased version of all class-names in quirks
mode in elements and selectors...
So, I took a look at this today, and wrote the patch above. It makes basically all of the slow paths seen in the profiles and locally go away.

I could be convinced to write a harder fix that would be a bit more general (storing the lower-cased tags / ids), but that looks a lot like a bigger can of worms due to DOMTokenList and such which _need_ to be case-sensitive, and also a little bit annoying because we need to store the original and the lowercased ids / classes in selectors, for which I'd need to ensure not to regress memory usage at least in standards mode or something.

WebKit does do this last bit though:

  http://webkit.crisal.io/webkit/rev/4a215c24151f81d78257072d3eff9e535382a2f6/Source/WebCore/css/CSSSelector.h#455

Anyway, this should help quite a lot, and depending on your feedback and (assuming it's positive) review feedback I may go for the harder / more annoying fix or not...

Dave, mind giving a try to one of the (optimized) builds for:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=91516ffc93e1d77fa96eae72b81a9e7f8e8dcf99

And confirming that it fixes the perf issues, and otherwise point me to the slowest bits? (I've just gone around poking at the website, so I could've missed stuff)
Assignee: nobody → emilio
Flags: needinfo?(emilio) → needinfo?(justdave)
Summary: Panorama appliance is exceedingly slow in Firefox (but fast in SafarI; ExtJS spent 30+ sec in doLayout) → Panorama appliance is exceedingly slow in Firefox (selector-matching in quirks-mode document)
Also, I didn't expect to write perf fixes to quirks mode documents in 2018!

If they still maintain that app, it may be worth poking them to port it to standards mode :)
Comment on attachment 9022706 [details]
Bug 1499170 - Add an atom bit to know whether we're ascii lowercase.

Nick, any strong opinion about the patch? Does it sound reasonable (apart from various XXX comments that probably need to be tweaked)?

I feel a little bad taking another bit from the atom string length, but also didn't want to block this on killing the HTML5Atoms (maybe I should though?).

The background here is that selector-matching on classes and IDs uses case-insensitive comparison for quirks-mode, which means that in this particular huge quirks mode page it just takes too long. This fixes the perf issue for the (by far) common case of both strings being ASCII lowercase.
Attachment #9022706 - Flags: feedback?(n.nethercote)
I filed ticket 01015765 with Palo Alto to submit the feature request to get the web interface ported to standards-compliant.

I'll try out your build later tonight when I'm in less of a rush with other stuff.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)
> Dave, mind giving a try to one of the (optimized) builds for:
>  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=91516ffc93e1d77fa96eae72b81a9e7f8e8dcf99
> 
> And confirming that it fixes the perf issues, and otherwise point me to the
> slowest bits? (I've just gone around poking at the website, so I could've
> missed stuff)

I've not used Treeherder before.  How do I download? I assumed the links next to the build names would be a download, but it doesn't seem to give me anything so I must be clicking in the wrong place.
nevermind, I found a Mac OS "opt" one that had a target.dmg artifact uploaded which seems to be it.
Oh, heck yeah, that's faster. :-)

The one spot where it's still slightly pokey is opening a virtual router.  Go to the Network tab at the top, click on Virtual Routers in the list on the left, then in the "Template" dropdown at the top, pick anything that starts with "Site".  Then click on "default" in the table, it should open a div layer dialog box (this is actually what I was describing in the initial description above).  It took 15 to 30 seconds before, and now takes 5 to 6 seconds.  But it's still slower than Safari that does it in 3. :-)  Probably not a big deal at this point (6 seconds is already a HUGE improvement) but you're dying of curiosity... :-)
Flags: needinfo?(justdave)
but *if* you're dying of curiosity....  I lost a word there somewhere
Comment on attachment 9022706 [details]
Bug 1499170 - Add an atom bit to know whether we're ascii lowercase.

Seems reasonable. I have a few nits in the Phabricator review.

If you feel motivated to remove HTML5 atoms that would be *great*. Bug 1392185 has two patches that pretty much work. However, the second one can't be used as is, because it modifies the HTML5 parser code, and that code is actually generated by another program in a different repo. So the generator needs to be modified to produce code that matches that in the second patch.
Attachment #9022706 - Flags: feedback?(n.nethercote) → feedback+
Alright, I'll take a look tomorrow, I'll cleanup the patch, and maybe profile the bit described in comment 23, and maybe even try to kill the HTML5 atoms :-)

I'll push a talos run just to ensure that this doesn't terribly regress anything else, otherwise I need to think on how to avoid it.
Flags: needinfo?(emilio)
I'll wait for bug 1392185 in order to cleanup the patch, since I took it and that way I can avoid changing the maximum atom length.
Depends on: 1392185
Blocks: 1505116
Blocks: 1505117
Attachment #9022706 - Attachment description: Bug 1499170 - [WIP] Add an atom bit to know whether we're ascii lowercase. → Bug 1499170 - Add an atom bit to know whether we're ascii lowercase.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/2655a3d8dcd9
Add an atom bit to know whether we're ascii lowercase. r=njn
https://hg.mozilla.org/mozilla-central/rev/2655a3d8dcd9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: needinfo?(emilio)
Performance Impact: --- → P2
Whiteboard: [qf:p2:responsiveness]
You need to log in before you can comment on or make changes to this bug.