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

Status: RESOLVED FIXED

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

Description

[Dave Miller [:justdave]]

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.

Comment 1

[Dave Miller [:justdave]]

Peformance profile while opening a virtual router dialog:
https://perfht.ml/2OYGGEU

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Dave Miller [:justdave] ([e-mail]) 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.

Comment 3

[:Gijs (he/him)]

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?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 6

[Cameron McCormack (:heycam)]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 8

[Dave Miller [:justdave]]

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.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Unfortunately, Palo Alto firewalls don't offer an online live demo to play around (couldn't find it by causally searching).

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I happen to be a moco employee... I can figure out how to set up the VPN sometime this week :)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

I have a VPN setup now (my moco ldap thing is ealvarez@). Any chance I can see the website?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Thanks! I'll take a look tomorrow :)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

> Sorry, you do not have permission to access panorama.mozilla.net. Please contact [e-mail] if you should have access. 

:/

Comment 14

[Dave Miller [: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. :-)

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Alright, will try again! :)

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1499170 - Add an atom bit to know whether we're ascii lowercase.

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

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

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 19

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 20

[Dave Miller [:justdave]]

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.

Comment 21

[Dave Miller [:justdave]]

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

Comment 22

[Dave Miller [:justdave]]

nevermind, I found a Mac OS "opt" one that had a target.dmg artifact uploaded which seems to be it.

Comment 23

[Dave Miller [:justdave]]

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

Comment 24

[Dave Miller [:justdave]]

but *if* you're dying of curiosity....  I lost a word there somewhere

Comment 25

[Nicholas Nethercote [inactive]]

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.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 27

[Emilio Cobos Álvarez (: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.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2655a3d8dcd9
Add an atom bit to know whether we're ascii lowercase. r=njn

Comment 29

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2655a3d8dcd9