1332525 - stylo: Style sharing cache not working

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

NI emilio to investigate!

Comment 2

[Bobby Holley (:bholley)]

Attachment: Improve logging a tiny bit.

Please fold this into your patch if you can, since we might as well get it
in-tree.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

I'm trying to land a preliminar patch for this at https://github.com/servo/servo/pull/15160

Comment 4

[Emilio Cobos Álvarez [:emilio]]

(Will probably land the logging patch as part of the rest of the style sharing cache changes)

Comment 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

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.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Do a better job of detecting where there are transitions. v1

The current code thinks that every element has transitions.

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a1fc20bbb85a59e421777e52ce5c241eb0db625

Comment 9

[Boris Chiou [:boris]]

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

Comment 10

[Boris Chiou [:boris]]

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

Comment 11

[Bobby Holley (:bholley)]

Attachment: Do a better job of detecting where there are transitions. v2

This is the suggested patch, but the function always returns true.

Comment 12

[Boris Chiou [:boris]]

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;

Comment 13

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1177639bb7b18633740312e1659bb4b4f6990f1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Do a better job of detecting where there are transitions. v3

The current code thinks that every element has transitions.

Comment 15

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/16358