1407864 - Parsed selector cache for selectors-api is subject to thrashing between Gecko and Servo selectors

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Significant performance regression.

This is causing the slowdown reported in bug 1405899 comment 6 and bug 1405899 comment 9.

Profile shows that we're spending way more time selector-parsing after bug 1404897.  That's because the relevant extension script (in uBlock Origin) has this bit (in the processHighHighGenerics function in js/contentscript.js):

            var matchesProp = vAPI.matchesProp,
                nodes = surveyPhase3Nodes,
                i = nodes.length, node;
            while ( i-- ) {
                node = nodes[i];
                if (
                    node[matchesProp](highGenerics.hideHighSimple) ||
                    node.querySelector(highGenerics.hideHighSimple) !== null
                ) {

matchesProp is is dealing with possibly-prefixed "matches".  So what this is really doing is taking a single selector string and passing it to matches(), then to querySelector().

After the patch in bug 1404897, what that will do is parse the selector as a servo selector (for matches()), cache it.  Then it will go to parse it as a gecko selector (for querySelector()), get a cache miss because the cached thing is a servo selector, parse as a Gecko selector, override the cache entry, etc.  We're spending 95+% of the time under matches/querySelector parsing selectors in the linked profiles.

Plausible fixes... we could switch querySelector and company over to servo selectors too.  We could add an isServo boolean to the cache key.  Or we could make the cache value be able to hold both gecko and servo selectors.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Ugh, that's stupid, yeah... Thanks for profiling it. Can look into it tomorrow if you want.

Comment 2

[Boris Zbarsky [:bzbarsky]]

gwarser did the profiling.  I just looked at his nice profiles.  ;)

Comment 3

[Kacper Michajłow [:kasper93]]

If you like profiles, here is one more from the first test page gwarser linked. Started profile after loading page and scrolled few times. Thumbnails are dynamically reloaded and each time uBO recheck all of them against the selector.

Stylo Enabled : https://perfht.ml/2xA3vCa
Stylo Disabled: https://perfht.ml/2xzI7Np
Selector being matched: https://gist.github.com/kasper93/028a0df358720f2c27a81b4193724bac

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1407864: Do not thrash the selector cache when parsing the same selector with different backend.

Since the presence of an entry with a null selector is different for Gecko and
Servo, this seemed easier, and mimics nsLayoutStyleSheetCache.

Also, this is going away soon anyway as soon as I get to implement the rest of
the methods for stylo.

Review commit: https://reviewboard.mozilla.org/r/188660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188660/

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8917731 [details]
Bug 1407864: Do not thrash the selector cache when parsing the same selector with different backend.

https://reviewboard.mozilla.org/r/188660/#review193988

::: dom/base/nsIDocument.h:3209
(Diff revision 1)
>  
>  private:
>    mutable std::bitset<eDeprecatedOperationCount> mDeprecationWarnedAbout;
>    mutable std::bitset<eDocumentWarningCount> mDocWarningWarnedAbout;
> +
>    // Lazy-initialization to have mDocGroup initialized in prior to mSelectorCache.

Nit: s/mSelectorCache/these SelectorCaches/ or similar.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8917731 [details]
Bug 1407864: Do not thrash the selector cache when parsing the same selector with different backend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188660/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06ca0845ca1e
Do not thrash the selector cache when parsing the same selector with different backend. r=heycam

Comment 8

[u603568]

> Thumbnails are dynamically reloaded and each time uBO recheck all of them against the selector.

For what it's worth, as mentioned in the GitHub repo[1], this code won't afflict Firefox anymore when the code is refactored such that uBO will depend strictly on user styles to enforce "display: none !important;". This code is actually already useless on Firefox given that uBO already relies on user stylesheet on Firefox, but removing that code path requires refactoring for all platforms (Chromium, Firefox) which is what I want out before Firefox 57 stable is released.[2]


[1] https://github.com/gorhill/uBlock/issues/3111#issuecomment-335893602
[2] https://github.com/gorhill/uBlock/issues/2984

Comment 9

[Emilio Cobos Álvarez (:emilio)]

57 is not affected, the Element::Matches patch landed after that.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/06ca0845ca1e