Open
Bug 1422416
Opened 6 years ago
Updated 1 year ago
stylo: No need to mark uncacheable for style adjustment based on reset properties only
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
Tracking
()
NEW
Tracking | Status | |
---|---|---|
firefox59 | --- | affected |
People
(Reporter: xidorn, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
Currently we mark styles uncacheable for rule cache as soon as style adjuster changes anything. However, this isn't really necessary. There are cases where adjustment is purely based on some other reset properties and the adjustment itself is idempotent (actually, it is likely a bug if any of the adjustment is not idempotent). In that case, we should be able to allow caching reset properties. We can put rule condition into StyleAdjuster for this, and invoke set_uncacheable when necessary.
Comment 1•6 years ago
|
||
This is not true, right? (see the modified_reset check) https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/servo/components/style/properties/properties.mako.rs#3530
Assignee | ||
Comment 2•6 years ago
|
||
What is not true? I was actually looking at modified_reset check when I filed this bug, because I don't think it is necessary to avoid caching in all those cases.
Assignee | ||
Comment 3•6 years ago
|
||
By "changes anything" in comment 0, I mean, when it changes any reset property. This seems to be too coarse-grained to me.
Comment 4•6 years ago
|
||
We either cache all reset structs or none, so it looks to me it's necessary?
Assignee | ||
Comment 5•6 years ago
|
||
If we don't cache all reset structs together, we cannot cache adjustment because there could be inter-dependency between different reset structs. That's the case for Gecko. But since we cache all reset structs together in Stylo, we should be able to cache post-adjustment result.
Comment 6•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #5) > If we don't cache all reset structs together, we cannot cache adjustment > because there could be inter-dependency between different reset structs. > That's the case for Gecko. > > But since we cache all reset structs together in Stylo, we should be able to > cache post-adjustment result. I see what you mean... Is that the case that there's not inherited -> reset dependence? If it is, that doesn't work in presence of inheritance. And that seems to be the case already... For example, if we have two elements with different writing mode, they may match the same rules, but the reset structs end up being different (due to adjust_for_writing_mode).
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6) > Is that the case that there's not inherited -> reset dependence? If it is, > that doesn't work in presence of inheritance. And that seems to be the case > already... There are definitely some adjustments which are uncacheable, and we would need to mark them explicitly. But there are many adjustments which are cacheable. I intend to improve caching for this. > For example, if we have two elements with different writing mode, they may > match the same rules, but the reset structs end up being different (due to > adjust_for_writing_mode). This would be a case where we need to tell callsite of style adjuster that we have done some adjustment which makes reset structs not cacheable. adjust_for_writing_mode depends on parent's writing-mode to change display, so naturally it makes reset structs not cacheable.
Assignee: nobody → xidorn+moz
Assignee | ||
Comment 8•6 years ago
|
||
I have a patch for this now, and have pushed to try. We will see whether that would break anything.
Updated•6 years ago
|
Priority: -- → P2
Comment hidden (off-topic) |
Comment 10•1 year ago
|
||
This would need an updated patch and measurement.
Severity: normal → N/A
Flags: needinfo?(emilio)
Priority: P2 → P3
Assignee | ||
Comment 11•1 year ago
|
||
I think the bot is simply being silly. I definitely logined in the last month, actually I just made a comment yesterday.
Comment 12•1 year ago
|
||
Sorry, there was a problem with the detection of inactive users. I'm reverting the change.
Assignee: nobody → xidorn+moz
You need to log in
before you can comment on or make changes to this bug.
Description
•