Closed
Bug 1374135
Opened 7 years ago
Closed 5 years ago
stylo: set NodeHasRelevantHoverRules or figure out how to avoid doing so
Categories
(Core :: CSS Parsing and Computation, defect, P4)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: bzbarsky, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
See bug 1373798 comment 9.
Historical context: this was added in bug 732667. That bug has steps to reproduce the slowness, assuming github has not changed their styles in the last 5+ years. ;)
In particular the Bloom filter does _not_ help in the situation described in that bug. But maybe the changes for bug 1368240 make this unnecessary. We would need to test.
Reporter | ||
Comment 1•7 years ago
|
||
This one needs measurement and investigation before we can figure out the priority...
Updated•7 years ago
|
Priority: -- → P2
Comment 2•7 years ago
|
||
This may still be a problem in some cases.
There is a testcase where :hover does match something, and Stylo is significantly slower than Gecko and Blink.
There is another testcase where we have an additional rule which matches nothing, and Stylo and Gecko behave equally poor, while Blink is significantly faster.
Comment 3•7 years ago
|
||
This is the first testcase, where #test element has lots of children, there are lots of rules matching its children, and there is rule with selector #test:hover.
You can see difference by hovering the pointer on and off the blue square. The square should turn green when hovering.
With stylo disabled, it responds immediately, while with stylo enabled, on my machine, it takes tons of CPU for several seconds until it eventually changes.
Comment 4•7 years ago
|
||
The only difference in this testcase over testcase 1 is that, there is an additional rule against a nonexistent element.
Stylo and Gecko both perform poorly, but it seems to me that Gecko takes slightly shorter time, and much less CPU usage. (Stylo uses ~75% CPU * ~5s, while Gecko uses ~12% CPU * ~4s)
Blink, on the contrary, can still respond instantly in this testcase.
Comment 5•7 years ago
|
||
I'm not sure how this testcase can translate to real life websites, but I guess this kind of pattern isn't that unusual, although the number wouldn't be this extreme in general.
Comment 6•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> Created attachment 8902540 [details]
> testcase 1
>
> This is the first testcase, where #test element has lots of children, there
> are lots of rules matching its children, and there is rule with selector
> #test:hover.
>
> You can see difference by hovering the pointer on and off the blue square.
> The square should turn green when hovering.
>
> With stylo disabled, it responds immediately, while with stylo enabled, on
> my machine, it takes tons of CPU for several seconds until it eventually
> changes.
This is... not the test-case I was expecting, and isn't because of this.
This is because we don't stop the cascade if only reset properties change but no children explicitly inherited from us, so we need to end up recalculating all the children's styles.
But that's totally not related to this bug.
Comment 7•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Created attachment 8902542 [details]
> testcase 2
>
> The only difference in this testcase over testcase 1 is that, there is an
> additional rule against a nonexistent element.
>
> Stylo and Gecko both perform poorly, but it seems to me that Gecko takes
> slightly shorter time, and much less CPU usage. (Stylo uses ~75% CPU * ~5s,
> while Gecko uses ~12% CPU * ~4s)
>
> Blink, on the contrary, can still respond instantly in this testcase.
Stylo takes a lot of time on this because of the same thing, not because of poor invalidation. I'll post a test-case where the invalidation machinery can be seen. The test-case that's coming responds instantly on stylo because we only scan the state dependencies in the selectormap, which is fast.
I'd say we don't need that flag. We can look into stopping the cascade if no children explicitly inherits from us, though.
Comment 8•7 years ago
|
||
Another option is to put an intermediate <div> in between, too.
Anyway, this shows how Stylo and Blink are able to determine that the .nonexists rule doesn't affect any children, and avoid restyling the thing, while Gecko does blow away the subtree.
This is still not quite related to this bug, because the node has indeed relevant hover rules applying to it.
The way to make this bug apply is to remove the #test:hover + div rule, which would make the node not having relevant hover rules.
When hovering over the element, neither Stylo, nor Gecko, nor Blink, use high amounts of CPU.
Updated•7 years ago
|
Attachment #8902552 -
Attachment mime type: text/plain → text/html
Comment 9•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> When hovering over the element, neither Stylo, nor Gecko, nor Blink, use
> high amounts of CPU.
This is with the rule removed, of course. With the test-case as is, Gecko spends a lot of time which Blink and stylo don't.
Comment 10•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> The way to make this bug apply is to remove the #test:hover + div rule,
> which would make the node not having relevant hover rules.
Actually, I don't think that's true, given Gecko doesn't apply that optimization for stuff that can match other mutable state... See bug 758885.
I personally think that we can avoid worrying about this flag, and recover that bit once non-stylo is gone.
Comment 11•7 years ago
|
||
Priority: P2 → P4
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → fix-optional
Comment 12•7 years ago
|
||
status-firefox59:
--- → ?
Reporter | ||
Comment 13•5 years ago
|
||
NodeHasRelevantHoverRules is gone as of bug 1484478.
You need to log in
before you can comment on or make changes to this bug.
Description
•