Open
Bug 1406622
Opened 7 years ago
Updated 2 years ago
stylo: use invalidation framework to handle restyles needed in response to content insertion/removal
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Core
CSS Parsing and Computation
Tracking
()
People
(Reporter: heycam, Assigned: emilio)
References
(Blocks 3 open bugs)
Details
(Keywords: perf:responsiveness)
Currently we respond to content modifications needing to restyle elements due to slow selector flags by posting restyles. For example, when we append a new element to a parent that is marked as having a child that needs :last-child matched, we'll post an eRestyle_Subtree on the old last child. It seems like we should be able to use invalidations to do more targeted restyling.
Assignee | ||
Comment 1•7 years ago
|
||
Yes, indeed! I thought about this before, and this would allow us to avoid the selector_flags_map, and the flags_setter, which is nice.
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 2•7 years ago
|
||
So I thought a bit more about how to do this, and of course it's not an easy fix, but it's not impossible either.
The problem is that we can't invalidate style synchronously during the DOM mutation, because that can get out of sync with class changes and what not.
So the proper way to fix this, I think, is to teach the snapshot stuff about positional pseudo-classes. For that, we could make the snapshots have an "old next / prev" sibling, that would allow us to reconstruct the old sibling chain.
So it'd be something like, for positional pseudo-classes, snapshot the prev and next siblings on DOM insertion / removal. Then the invalidation machinery would take over and invalidate other stuff if needed as it does for attribute changes.
For :empty / :-moz-only-whitespace it's a bit more annoying, because we need to snapshot the parent, but I think it's not much harder than that / making the snapshot aware of whether it was empty.
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> For :empty / :-moz-only-whitespace it's a bit more annoying, because we need
> to snapshot the parent, but I think it's not much harder than that.
Hmm, well, it might, because we'd need to compute accurately whether :empty / :-moz-only-whitespace have changed...
Assignee | ||
Comment 4•7 years ago
|
||
I think the only way to fix this in a sane way is either snapshotting previous parent / siblings of the DOM element, or just ditching the "invalidate during the traversal" stuff and do it sync. But that's kind of a pity.
Updated•6 years ago
|
Blocks: layout-perf
Assignee | ||
Comment 5•6 years ago
|
||
A somewhat mixed alternative would be to collect the invalidations sync, and invalidate during the traversal... That may be a good trade-off I guess.
Assignee | ||
Comment 6•6 years ago
|
||
I have better ideas I think...
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=134f68176227a35007c081873dc34abbbf6caee7 is the try run.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=025022534e4252dd20852d76af2a7ef161e6ea2a&framework=1&selectedTimeRange=172800 is the talos push.
I didn't remove the selector flags setters and stuff but I should... I left them just in case they were needed to make stuff faster, though so far talos hasn't showed anything ridiculous.
This should fix bug 1480477 & co. There's a bunch of tricky invalidation test-cases I want to land.
Also I think I haven't handled :empty invalidation yet, but that should be trivial compared to :nth-child & co.
Assignee: nobody → emilio
Blocks: 1480477
Assignee | ||
Comment 8•6 years ago
|
||
Also, some more cleanup can be done too afterwards.
Assignee | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78f86787b8caaf894c478a32636e4998aa521fec has a couple more fixes, still not 100% right.
Assignee | ||
Comment 10•6 years ago
|
||
Looks like I have some perf work to do if I really want to get rid of the slow selector flags... Stylebench isn't happy about this:
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=e0bc980ed752fdc00a810ad09ac942351f35cd22
Assignee | ||
Comment 11•6 years ago
|
||
Boris, friendly ping regarding your feedback on the style invalidation? Let me know if you don't have cycles to look into it and I'll just write patches :P
Flags: needinfo?(bzbarsky)
Comment 13•6 years ago
|
||
Sorry, I've been on unexpected PTO most of this week... I won't really be back to work until Tuesday morning. I will definitely try to look at this then.
Updated•6 years ago
|
Whiteboard: [qf:p1:f64]
Comment 14•6 years ago
|
||
--> retriaging as [qf:p2:responsiveness] since qf:p1 is now reserved for pageload, and it sounds like this bug is mostly about reacting to dynamic changes.
Whiteboard: [qf:p1:f64] → [qf:p2:responsiveness]
Comment 15•5 years ago
|
||
I'm sorry I never got to the invalidation question here. :( I don't think I'm going to manage it in the next few days either...
Flags: needinfo?(bzbarsky)
Updated•3 years ago
|
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•