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)

53 Branch
defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- ?

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.
This one needs measurement and investigation before we can figure out the priority...
Priority: -- → P2
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.
Attached file testcase 1 (deleted) —
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.
Attached file testcase 2 (deleted) —
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.
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.
(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.
(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.
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.
Attachment #8902552 - Attachment mime type: text/plain → text/html
(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.
(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.
Filed bug 1395227 about the missing optimization in comment 6. Downgrading this bug per comment 10.
Priority: P2 → P4

NodeHasRelevantHoverRules is gone as of bug 1484478.

Status: NEW → RESOLVED
Closed: 5 years ago
Depends on: 1484478
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: