stylo: Fix up the style sharing cache

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
There are various broken and suboptimal bits that I have patches for.
(Assignee)

Comment 1

2 years ago
Created attachment 8860584 [details] [diff] [review]
Part 1 - Don't skip the style sharing cache if an element is affected by state. v1

All elements end up with AFFECTED_BY_STATE right now, presumably due to the UA
sheet, and some non-TS pseudo-classes we've added, which means that we currently
never insert anything into the cache.

MozReview-Commit-ID: 5IU4qrjeJFy
Attachment #8860584 - Flags: review?(emilio+bugs)
(Assignee)

Comment 2

2 years ago
Created attachment 8860585 [details] [diff] [review]
Part 2 - Remove unused StyleRelations. v1

MozReview-Commit-ID: ed0NiuY9Ek
Attachment #8860585 - Flags: review?(emilio+bugs)
(Assignee)

Comment 3

2 years ago
Created attachment 8860586 [details] [diff] [review]
Part 3 - Reorganize bits and make the flags usize. v1

These are never stored persistently anywhere, and I'm pretty sure it's slower
for the compiler/CPU to operate on non-word-sized flags.

MozReview-Commit-ID: LQNsJbUsw85
Attachment #8860586 - Flags: review?(emilio+bugs)
(Assignee)

Comment 4

2 years ago
Created attachment 8860587 [details] [diff] [review]
Part 4 - Make it clearer that we never insert elements with preshints into the cache. v1

MozReview-Commit-ID: 9bdc0sNZeEy
Attachment #8860587 - Flags: review?(emilio+bugs)
(Assignee)

Comment 5

2 years ago
Created attachment 8860588 [details] [diff] [review]
Part 5 - Share styles between cousins if the parent styles were shared. v1

MozReview-Commit-ID: 1QFCSzC5xvk
Attachment #8860588 - Flags: review?(emilio+bugs)
(Assignee)

Updated

2 years ago
Attachment #8860586 - Attachment description: Part 2 - Reorganize bits and make the flags usize. v1 → Part 3 - Reorganize bits and make the flags usize. v1
(Assignee)

Updated

2 years ago
Attachment #8860587 - Attachment description: Part 3 - Make it clearer that we never insert elements with preshints into the cache. v1 → Part 4 - Make it clearer that we never insert elements with preshints into the cache. v1
Comment on attachment 8860584 [details] [diff] [review]
Part 1 - Don't skip the style sharing cache if an element is affected by state. v1

Review of attachment 8860584 [details] [diff] [review]:
-----------------------------------------------------------------

Without this, what prevents an element matching :hover to share style with a sibling that doesn't, for example?

I'd rather see what's going on (presumably the state flags are those of the :dir stuff, or similar), and explicitly whitelist/check those are the same instead.
Attachment #8860584 - Flags: review?(emilio+bugs)
Comment on attachment 8860585 [details] [diff] [review]
Part 2 - Remove unused StyleRelations. v1

Review of attachment 8860585 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8860585 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8860584 [details] [diff] [review]
Part 1 - Don't skip the style sharing cache if an element is affected by state. v1

Review of attachment 8860584 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, right, we have the get_state() != candidate.get_state() check, so this is fine. We can probably make it more granular if need be.
Attachment #8860584 - Flags: review+
Comment on attachment 8860586 [details] [diff] [review]
Part 3 - Reorganize bits and make the flags usize. v1

Review of attachment 8860586 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think making them usize should be a big deal performance-wise btw, you'd just use smaller registers, which are as fast as word-sized ones.

Anyway, this is fine.
Attachment #8860586 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8860587 [details] [diff] [review]
Part 4 - Make it clearer that we never insert elements with preshints into the cache. v1

Review of attachment 8860587 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/matching.rs
@@ +289,5 @@
>          }
>  
> +        // Make sure we noted any presentational hints in the StyleRelations.
> +        if cfg!(debug_assertions) {
> +            let mut hints = vec![];

You can use ForgetfulSink here too. No big deal though.
Attachment #8860587 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8860588 [details] [diff] [review]
Part 5 - Share styles between cousins if the parent styles were shared. v1

Review of attachment 8860588 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/matching.rs
@@ +107,5 @@
> +        (Some(f), Some(s)) => (f, s),
> +        _ => return false,
> +    };
> +
> +    let eq = ::arc_ptr_eq(a.borrow_data().unwrap().styles().primary.values(),

I'd just remove this variable, I don't think it provides anything specially useful.
Attachment #8860588 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 13

2 years ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Comment on attachment 8860588 [details] [diff] [review]
> Part 5 - Share styles between cousins if the parent styles were shared. v1
> 
> Review of attachment 8860588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/matching.rs
> @@ +107,5 @@
> > +        (Some(f), Some(s)) => (f, s),
> > +        _ => return false,
> > +    };
> > +
> > +    let eq = ::arc_ptr_eq(a.borrow_data().unwrap().styles().primary.values(),
> 
> I'd just remove this variable, I don't think it provides anything specially
> useful.

It's needed to avoid borrow checker issues.
(Assignee)

Comment 14

2 years ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> You can use ForgetfulSink here too. No big deal though.

Good point.
(Assignee)

Comment 15

2 years ago
Created attachment 8860677 [details] [diff] [review]
Part 6 - Disable the style sharing cache on opt builds to avoid regressing Talos by matching too many revalidation selectors. v1

I plan to fix bug 1358693 next, but want to get this landed in the mean time
without regressing our performance numbers (which would happen, because we
currently don't use the setyle sharing cache at all right now because of the
issue fixed by Part 1).

MozReview-Commit-ID: 10qSho1gXjw
Attachment #8860677 - Flags: review?(emilio+bugs)
Attachment #8860677 - Flags: review?(emilio+bugs) → review+
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.