1356148 - stylo: Coalesce duplicate slow selector flags

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

The current setup blows up with the testcase at [1] modified to have the selector of "span + div".

[1] https://hg.mozilla.org/integration/autoland/file/6386d8f32e1d/testing/talos/talos/tests/perf-reftest/bloom-basic.html

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Optimize LRUCache by using a VecDeque. v1

We also reverse the order, so that we don't need iter.rev().

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Store slow selector flags in a hashmap. v1

This reduces the flag setting overhead on a pessimal from over a second to ~20ms.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8857876 [details] [diff] [review]
Part 1 - Optimize LRUCache by using a VecDeque. v1

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

::: servo/components/style/cache.rs
@@ +37,5 @@
>          self.entries.len()
>      }
>  
>      #[inline]
>      /// Touch a given position, and put it in the last item on the list.

Update the comment above this, please :)

::: servo/components/style/context.rs
@@ +8,5 @@
>  use animation::{Animation, PropertyAnimation};
>  use app_units::Au;
>  use bit_vec::BitVec;
>  use bloom::StyleBloom;
> +use cache::LRUCache;

This seems out of place, right?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8857877 [details] [diff] [review]
Part 2 - Store slow selector flags in a hashmap. v1

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

The commit message needs updating: "on a pessimal"? I guess you mean "pessimal situation" or something like that?

Anyway, with that and the following nits addressed. r=me

::: servo/components/style/context.rs
@@ +308,5 @@
> +        *f |= flags;
> +
> +        // Insert into the cache. We don't worry about duplicate entries,
> +        // which lets us avoid reshuffling.
> +        self.cache.insert((unsafe { SendElement::new(element)}, *f))

nit: Space between the paren and the brace.

@@ +318,5 @@
> +        for (el, flags) in self.map.drain() {
> +            unsafe { el.set_selector_flags(flags); }
> +        }
> +    }
> +}

Add a `Drop` implementation that debug_asserts that the map is empty? Or alternatively, just drain it on `Drop`?

@@ +400,5 @@
>          debug_assert!(self.current_element_info.is_none());
> +        debug_assert!(thread_state::get() == thread_state::LAYOUT);
> +
> +        // Apply any slow selector flags that need to be set on parents.
> +        self.selector_flags.apply_flags();

Yeah, presumably, just drain it on drop of the SelectorFlagsMap itself.

::: servo/components/style/gecko/wrapper.rs
@@ +701,5 @@
>          self.0 as *const _ == other.0 as *const _
>      }
>  }
>  
> +impl<'le> Eq for GeckoElement<'le> {}

Presumably you also need to do the same in the Servo side, but assuming it's the same, that's fine.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Comment on attachment 8857877 [details] [diff] [review]
> Part 2 - Store slow selector flags in a hashmap. v1
> 
> Review of attachment 8857877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The commit message needs updating: "on a pessimal"? I guess you mean
> "pessimal situation" or something like that?

:-P

> @@ +318,5 @@
> > +        for (el, flags) in self.map.drain() {
> > +            unsafe { el.set_selector_flags(flags); }
> > +        }
> > +    }
> > +}
> 
> Add a `Drop` implementation that debug_asserts that the map is empty? Or
> alternatively, just drain it on `Drop`?

I think there's value in having this logic in the same place as the SequentialTask dispatch logic. I'll add the debug-only drop.

Comment 8

[Bobby Holley (:bholley)]

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