1422416 - stylo: No need to mark uncacheable for style adjustment based on reset properties only

Status: NEW

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

Description

[Xidorn Quan [:xidorn] UTC+11]

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

[Emilio Cobos Álvarez (:emilio)]

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

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

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

[Emilio Cobos Álvarez (:emilio)]

We either cache all reset structs or none, so it looks to me it's necessary?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

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

[Emilio Cobos Álvarez (:emilio)]

(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).

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(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.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

I have a patch for this now, and have pushed to try. We will see whether that would break anything.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'.
:emilio, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

This would need an updated patch and measurement.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

I think the bot is simply being silly. I definitely logined in the last month, actually I just made a comment yesterday.

Comment 12

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.