stylo: Style sharing cache not working

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

One discovery in bug 1331848 and the logging in bug 1331856 is that the style sharing doesn't appear to be getting populated at all.

There are potentially several reasons for this. Emilio pointed out that the cache is optimized for parallel traversal, and probably doesn't do the right thing for sequential traversal. My logging also indicates that we never insert any elements into the cache because of "animations", which is probably wrong.
NI emilio to investigate!
Flags: needinfo?(emilio+bugs)
Created attachment 8828614 [details] [diff] [review]
Improve logging a tiny bit.

Please fold this into your patch if you can, since we might as well get it
in-tree.
I'm trying to land a preliminar patch for this at https://github.com/servo/servo/pull/15160
(Will probably land the logging patch as part of the rest of the style sharing cache changes)
This is probably p1 because it has a large impact on the performance measurements we'll get out of the style system. Let me know if it looks like it will take a lot of time and we can re-evaluate.
Priority: -- → P1
Assignee: nobody → emilio+bugs
Some initial cleanup at https://github.com/servo/servo/pull/15888.

I plan to make some improvements to the current cache before implementing the sequential logic.
Flags: needinfo?(emilio+bugs)
Depends on: 1354806
Created attachment 8856227 [details] [diff] [review]
Do a better job of detecting where there are transitions. v1

The current code thinks that every element has transitions.
Attachment #8856227 - Flags: review?(boris.chiou)
Assignee: emilio+bugs → bobbyholley
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a1fc20bbb85a59e421777e52ce5c241eb0db625
Depends on: 1354895

Comment 9

4 months ago
Comment on attachment 8856227 [details] [diff] [review]
Do a better job of detecting where there are transitions. v1

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

::: servo/components/style/properties/gecko.mako.rs
@@ +1894,5 @@
> +    /// Returns whether there are any transitions specified.
> +    pub fn specifies_transitions(&self) -> bool {
> +        use gecko_bindings::structs::nsCSSPropertyID_eCSSPropertyExtra_no_properties;
> +        !(self.gecko.mTransitionPropertyCount == 1 &&
> +          self.gecko.mTransitions[0].mProperty != nsCSSPropertyID_eCSSPropertyExtra_no_properties)

I still need to check the impact of this condition on Bug 1341372, but we filter out the property if [1]:
  property == eCSSPropertyExtra_no_properties ||
  property == eCSSPropertyExtra_variable ||
  property == eCSSProperty_UNKNOWN

So maybe we should add more conditions like that. Thanks.

[1] http://searchfox.org/mozilla-central/rev/c4fdb67bca7889e59af9dd99c604651a49c4aafa/layout/style/nsTransitionManager.cpp#663-665
Blocks: 1243581

Comment 10

4 months ago
Comment on attachment 8856227 [details] [diff] [review]
Do a better job of detecting where there are transitions. v1

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

::: servo/components/style/properties/gecko.mako.rs
@@ +1894,5 @@
> +    /// Returns whether there are any transitions specified.
> +    pub fn specifies_transitions(&self) -> bool {
> +        use gecko_bindings::structs::nsCSSPropertyID_eCSSPropertyExtra_no_properties;
> +        !(self.gecko.mTransitionPropertyCount == 1 &&
> +          self.gecko.mTransitions[0].mProperty != nsCSSPropertyID_eCSSPropertyExtra_no_properties)

Oops. I think it is OK to check only nsCSSPropertyID_eCSSPropertyExtra_no_properties (for none transition property.)

Here you only filter out "transition:none" case, but I think it's better to also check self.gecko.mTransitionPropertyCount > 0 (which means we have at least one transition property)

So how about something like this:

self.gecko.mTransitionPropertyCount > 0 &&
self.gecko.mTransitions[0].mProperty != nsCSSPropertyID_eCSSPropertyExtra_no_properties
Created attachment 8856875 [details] [diff] [review]
Do a better job of detecting where there are transitions. v2

This is the suggested patch, but the function always returns true.
Attachment #8856227 - Attachment is obsolete: true
Attachment #8856227 - Flags: review?(boris.chiou)

Comment 12

4 months ago
Comment on attachment 8856875 [details] [diff] [review]
Do a better job of detecting where there are transitions. v2

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

::: servo/components/style/properties/gecko.mako.rs
@@ +1894,5 @@
> +    /// Returns whether there are any transitions specified.
> +    pub fn specifies_transitions(&self) -> bool {
> +        use gecko_bindings::structs::nsCSSPropertyID_eCSSPropertyExtra_no_properties;
> +        self.gecko.mTransitionPropertyCount > 0 &&
> +        self.gecko.mTransitions[0].mProperty != nsCSSPropertyID_eCSSPropertyExtra_no_properties

ok, let's filter out this case:

if (self.gecko.mTransitionPropertyCount == 1 &&
    self.gecko.mTransiiton[0].mProperty == nsCSSPropertyID_eCSSPropertyExtra_all_properties &&
    self.gecko.mTransiiton[0].mDuration.max(0.0) + self.gecko.mTransiiton[0].mDelay <= 0.0f32 ) {
    return false;
}

return self.gecko.mTransitionPropertyCount > 0;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1177639bb7b18633740312e1659bb4b4f6990f1
Created attachment 8856923 [details] [diff] [review]
Do a better job of detecting where there are transitions. v3

The current code thinks that every element has transitions.
Attachment #8856923 - Flags: review?(boris.chiou)
Attachment #8856875 - Attachment is obsolete: true

Updated

4 months ago
Attachment #8856923 - Flags: review?(boris.chiou) → review+
https://github.com/servo/servo/pull/16358
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.