1336646 - stylo: Accumulate parent elements that need selector bits set on the ThreadLocalStyleContext

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

One issue we've discussed in the past is that there are certain node flags (NODE_HAS_SLOW_SELECTORS, possibly others) that we need to set during the traversal. The problem is that, by the time we're processing the child and determine that the parent needs the flag, we've lost exclusive access to the parent, and can't safely set the bits.

We've been pushing to stableize non-word-sized atomics as a solution to this. But I realized a few days ago that this isn't going to work. Even if Servo code always does atomic reads while checking those flags, we can't do that everywhere in Gecko, and our FFI usage means that we might be executing Gecko code (which does non-atomic nodeflag reads) while another thread does an atomic write from Servo. Tracking these cases down in static analysis would be a pain and probably impractical.

Thankfully, we now have a new piece of infrastructure that gives us a nice way to solve this problem: the very-low-overhead ThreadLocalStyleContext, and the corresponding machinery to access and join the data from each thread after the parallel traversal is complete. So I think we should just queue these elements up in a Vec<> during the traversal, and then set the flags from the main thread once the traversal is done.

We need this in order to implement the various ServoRestyleManager::Content{Inserted,Appended,Removed} methods correctly.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

> Even if Servo code always does atomic reads while checking those flags, we can't do that everywhere in Gecko, and our FFI usage means that we might be executing Gecko code (which does non-atomic nodeflag reads) while another thread does an atomic write from Servo. Tracking these cases down in static analysis would be a pain and probably impractical.

To be honest, I'm not too fond with the cost that this approach has (I think atomic writes should be ok, those flags are only used in the RestyleManager, which is main-thread only). We should be able to assert that !ServoRestyleManager::IsInServoTraversal(), or that we're on the main thread when accessing them.

If you still think atomic ops aren't going to work, maybe we can reduce the load to use the "build a Vec" approach work only when a parent has its children split into more than a single chunk of work. In the case all the children are in the same chunk, there's no possible write to those flags from other thread.

But I still believe an atomic OR operation is the best solution to this.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> > Even if Servo code always does atomic reads while checking those flags, we can't do that everywhere in Gecko, and our FFI usage means that we might be executing Gecko code (which does non-atomic nodeflag reads) while another thread does an atomic write from Servo. Tracking these cases down in static analysis would be a pain and probably impractical.
> 
> To be honest, I'm not too fond with the cost that this approach has

Not sure I see the cost. The memory cost doesn't matter because this is scoped to the traversal. And given 7ns atomic ops and 40ns allocation+deallocation pairs (starting at capacity 4 and then doubling), the vec approach ends up being cheaper when non-trivial numbers of elements (> 23) need flags set, even ignoring other operations that need to become atomic (but see below).

But more to the point, the overhead of O(num_elements_in_dom) vec pushes is totally negligible for overall performance of the style system.

the allocation cost will actually be _cheaper_ than atomic sets

> (I think
> atomic writes should be ok, those flags are only used in the RestyleManager,
> which is main-thread only).

It's not just those bits. I'm pretty sure every other read and write to the same memory also needs to be atomic to avoid UB. That means any servo code or gecko code that gets/sets node flags.

 We should be able to assert that
> !ServoRestyleManager::IsInServoTraversal(), or that we're on the main thread
> when accessing them.
> 
> If you still think atomic ops aren't going to work, maybe we can reduce the
> load to use the "build a Vec" approach work only when a parent has its
> children split into more than a single chunk of work. In the case all the
> children are in the same chunk, there's no possible write to those flags
> from other thread.

I don't think the extra complexity is worth it.

> 
> But I still believe an atomic OR operation is the best solution to this.

Comment 3

[Bobby Holley (:bholley)]

Also, we might as well take the opportunity to make this a bit more general, and have an |enum PostTraversalOperation|, which allows us to add more kinds of infrequent non-threadsafe tasks that we want to execute when the traversal is complete. I remember heycam wanted something like that for image loads, though I think he ended up taking a different approach.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ok, fine then, if you think the memory hit and post-processing is not going to be costly, that's fine by me then :)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> But more to the point, the overhead of O(num_elements_in_dom) vec pushes is
> totally negligible for overall performance of the style system.
> 
> the allocation cost will actually be _cheaper_ than atomic sets
> 
> > (I think
> > atomic writes should be ok, those flags are only used in the RestyleManager,
> > which is main-thread only).
> 
> It's not just those bits. I'm pretty sure every other read and write to the
> same memory also needs to be atomic to avoid UB. That means any servo code
> or gecko code that gets/sets node flags.

Oh, and you're totally right on this, I don't know how I didn't realize they were stored along all the other node flags, silly me. Then I guess I don't have any better idea :)

Comment 6

[Bobby Holley (:bholley)]

bz, am I correct that these flags never get unset on a node once they're set? That certainly makes things easier...

Comment 7

[Bobby Holley (:bholley)]

I wrote a partial patch for this, will finish it when I get more time.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Use the bloom filter for manual style resolves and pass a mutable StyleContext into match_element. v1

We need to do something here to avoid a double-borrow when passing a mutable
StyleContext to match_element. After some discussion on IRC, we decided that
building the bloom filter for this case is probably worthwhile.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 2 - Move opts check for statistics into should_dump. v1

This makes things more consistent between the parallel and sequential traversal drivers.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Apply selector flags during traversal. v1

I rearranged the TLS handling in parallel.rs to provide access to the tasks vec
before realizing that I could just handle it in the destructor. I decided to
keep the refactoring though, since it'll make it easier in the future if we ever
need to do other post-processing of TLS data.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8834155 [details] [diff] [review]
Part 1 - Use the bloom filter for manual style resolves and pass a mutable StyleContext into match_element. v1

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

::: servo/components/style/traversal.rs
@@ +338,1 @@
>          }

Let's add a |filter.assert_complete(element)| call here, or at lease under the push() branch, just to sanity-check it.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8834157 [details] [diff] [review]
Part 3 - Apply selector flags during traversal. v1

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

r-, see individual commits for details.

Mainly, the way to make this approach work is either propagating the context to rust-selectors, which seems a bit overkill, or exposing ElementFlags as an out param from selector matching instead of calling set_flags directly there, but StyleRelations is not the way to go about this, because it represents a completely different thing.

::: servo/components/style/context.rs
@@ +170,5 @@
>  
> +/// A task to be run in sequential mode on the parent (non-worker) thread. This
> +/// is used by the style system to queue up work which is not safe to do during
> +/// the parallel traversal.
> +pub enum SequentialTask<E: TElement> {

Since it's likely that all the tasks end up carrying an Element, what about:

struct SequentialTask<E> {
    element: E,
    kind: SequentialTaskKind,
};

I guess it can be done later without too much effort, assuming the claim I'm doing above is true, so maybe don't bother too much about it :).

@@ +263,5 @@
>          debug_assert!(self.current_element_info.is_none());
> +
> +        // Execute any enqueued sequential tasks.
> +        debug_assert!(thread_state::get().is_layout());
> +        for task in mem::replace(&mut self.tasks, Vec::new()).into_iter() {

nit: for task in self.tasks.drain(..) should have the same effect, and be a bit nicer :)

::: servo/components/style/dom.rs
@@ +325,5 @@
> +
> +    /// Sets selector flags, which indicate what kinds of selectors may have
> +    /// matched on this element and therefore what kind of work may need to
> +    /// be performed when DOM state changes.
> +    unsafe fn set_selector_flags(&self, _flags: SelectorFlags) {}

So, note that we're doing in servo from rust-selectors. Probably we should just remove it from there and use the same mechanism for both. Otherwise, this needs rust-selectors changes, see below.

Also the rust-selectors code already prevents you from setting the flag when you're not styling (i.e., for Element.prototype.matches and similar). This would not be relevant for stylo for now, but it will be eventually.

::: servo/components/style/gecko/selector_parser.rs
@@ +340,5 @@
> +
> +    /// Computes the selector flags for an element's parent given its style relations.
> +    pub fn for_parent(relations: &StyleRelations) -> Self {
> +        let mut flags: u32 = 0;
> +        if relations.contains(AFFECTED_BY_CHILD_INDEX) {

This is wrong, we can't use relations for this. Relations represent stuff about the rules that the element _matches_, but you need to set the flag for elements that _may match_.

We have the same flags in rust-selectors (ElementFlags), with a method that is intended for this very use case, so you should use it instead of StyleRelations.

::: servo/components/style/gecko/wrapper.rs
@@ +423,5 @@
>          self.flags() & (NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE as u32) != 0
>      }
> +
> +    unsafe fn set_selector_flags(&self, flags: SelectorFlags) {
> +        use ::gecko_bindings::structs::*;

nit: no need for the initial ::. Also, if you're only using NODE_ALL_SELECTOR_FLAGS, seems like importing that only would be preferable.

::: servo/components/style/matching.rs
@@ +42,5 @@
> +                            AFFECTED_BY_PRESENTATIONAL_HINTS);
> +
> +    if result {
> +        // We don't currently allow elements whose StyleRelations imply selector
> +        // flags to be inserted into the cache, because we don't cache the

This should go away per my previous comment.

::: servo/components/style/parallel.rs
@@ +82,5 @@
> +    // Grab the TLS slots.
> +    let slots = unsafe { tls.unsafe_get() };
> +
> +    // Iterate over the slots.
> +    for slot in slots.iter() {

I'm not totally sure I follow this refactor? Seems like we'd be doing this loop completely unnecessarily if we don't need to dump statistics? Thought maybe I'm just missing something, huh.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Comment on attachment 8834157 [details] [diff] [review]
> Part 3 - Apply selector flags during traversal. v1
> 
> Review of attachment 8834157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r-, see individual commits for details.
> 
> Mainly, the way to make this approach work is either propagating the context
> to rust-selectors, which seems a bit overkill, or exposing ElementFlags as
> an out param from selector matching instead of calling set_flags directly
> there, but StyleRelations is not the way to go about this, because it
> represents a completely different thing.

Doh! That's what I get for not looking a few lines below StyleRelations. It's unfortunate that I needed to rework this, but nice that all the infrastructure is already there!

> 
> ::: servo/components/style/context.rs
> @@ +170,5 @@
> >  
> > +/// A task to be run in sequential mode on the parent (non-worker) thread. This
> > +/// is used by the style system to queue up work which is not safe to do during
> > +/// the parallel traversal.
> > +pub enum SequentialTask<E: TElement> {
> 
> Since it's likely that all the tasks end up carrying an Element, what about:
> 
> struct SequentialTask<E> {
>     element: E,
>     kind: SequentialTaskKind,
> };

Well, we still need to store task-specific data, and it's not clear to me that every kind of task would necessarily have an Element. So I'll leave as-is.

> 
> @@ +263,5 @@
> >          debug_assert!(self.current_element_info.is_none());
> > +
> > +        // Execute any enqueued sequential tasks.
> > +        debug_assert!(thread_state::get().is_layout());
> > +        for task in mem::replace(&mut self.tasks, Vec::new()).into_iter() {
> 
> nit: for task in self.tasks.drain(..) should have the same effect, and be a
> bit nicer :)

k.

> ::: servo/components/style/parallel.rs
> @@ +82,5 @@
> > +    // Grab the TLS slots.
> > +    let slots = unsafe { tls.unsafe_get() };
> > +
> > +    // Iterate over the slots.
> > +    for slot in slots.iter() {
> 
> I'm not totally sure I follow this refactor? Seems like we'd be doing this
> loop completely unnecessarily if we don't need to dump statistics? Thought
> maybe I'm just missing something, huh.

Yeah that's fair - I forgot that this patch makes the traversal unconditional. I'll just revert it - we can always pull the patch out of this bug if we need it again.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 3 - Apply selector flags during traversal. v2

Let me know if you're comfortable reviewing the rust-selectors changes, or if
I should flag Simon.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8834227 [details] [diff] [review]
Part 3 - Apply selector flags during traversal. v2

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

The approach looks good, though I would want to take another look at this after the replies to my comments.

::: servo/components/script/dom/element.rs
@@ +130,5 @@
>      attr_list: MutNullableJS<NamedNodeMap>,
>      class_list: MutNullableJS<DOMTokenList>,
>      state: Cell<ElementState>,
> +    #[ignore_heap_size_of = "bitflags defined in rust-selectors"]
> +    selector_flags: Cell<ElementSelectorFlags>,

Maybe describe how these are set and why they're not atomic? It might not be obvious for someone working in the DOM.

@@ +350,5 @@
>      fn namespace(&self) -> &Namespace;
>      fn get_checked_state_for_layout(&self) -> bool;
>      fn get_indeterminate_state_for_layout(&self) -> bool;
>      fn get_state_for_layout(&self) -> ElementState;
> +    fn insert_selector_flags(&self, flags: ElementSelectorFlags);

You probably also want a way to get the selector flags so you can avoid the churn of inserting them if it has already been done.

@@ +719,5 @@
>      }
>  
>      #[inline]
>      #[allow(unsafe_code)]
> +    fn insert_selector_flags(&self, flags: ElementSelectorFlags) {

I think you can assert here that thread_state::get() == LAYOUT, (note the equals, not the contains(), which would include layout workers).

@@ +1975,4 @@
>          match SelectorParser::parse_author_origin_no_namespace(&selectors) {
>              Err(()) => Err(Error::Syntax),
>              Ok(selectors) => {
> +                Ok(matches(&selectors.0, &Root::from_ref(self), None, &mut _flags))

What we do for the relations is just &mut ElementSelectorFlags::empty(), which is kind of self-explanatory. Does that not work for some reason?

::: servo/components/script/dom/node.rs
@@ +321,5 @@
>          // TODO(cgaebel): Is it worth it to build a bloom filter here
>          // (instead of passing `None`)? Probably.
>          self.iterator.by_ref().filter_map(|node| {
>              if let Some(element) = Root::downcast(node) {
> +                let mut _flags = ElementSelectorFlags::empty(); // Ignored.

Same, use &mut ElementSelectorFlags::empty() if possible.

::: servo/components/style/context.rs
@@ +179,5 @@
> +impl<E: TElement> SequentialTask<E> {
> +    /// Executes this task.
> +    pub fn execute(self) {
> +        use self::SequentialTask::*;
> +        debug_assert!(thread_state::get().is_layout());

debug_assert!(!thread_state::get().is_worker());

@@ +261,5 @@
>      fn drop(&mut self) {
>          debug_assert!(self.current_element_info.is_none());
> +
> +        // Execute any enqueued sequential tasks.
> +        debug_assert!(thread_state::get().is_layout());

This is kind of duplicated, but if you assert this, assert also !is_worker()

::: servo/components/style/dom.rs
@@ +324,5 @@
>      fn skip_root_and_item_based_display_fixup(&self) -> bool;
> +
> +    /// Sets selector flags, which indicate what kinds of selectors may have
> +    /// matched on this element and therefore what kind of work may need to
> +    /// be performed when DOM state changes.

Note how they're set here too, I think it's important if you wonder why this method is marked unsafe.

::: servo/components/style/matching.rs
@@ +34,5 @@
>  #[inline]
>  fn relations_are_shareable(relations: &StyleRelations) -> bool {
>      use selectors::matching::*;
>      !relations.intersects(AFFECTED_BY_ID_SELECTOR |
> +                          AFFECTED_BY_CHILD_INDEX |

if this intentional? If so, why? and why is it right? Given we could still cache styles that wouldn't match child index selectors but could?

@@ +631,5 @@
>          }
>  
> +        // Apply the selector flags.
> +        let self_flags = flags.for_self();
> +        if self_flags.is_empty() {

You mean if !self_flags.is_empty(), right?

@@ +635,5 @@
> +        if self_flags.is_empty() {
> +            unsafe { self.set_selector_flags(self_flags); }
> +        }
> +        let parent_flags = flags.for_parent();
> +        if parent_flags.is_empty() {

if !parent_flags.is_empty()? Actually, I believe you want to check that |(p.get_flags() & ALL_SELECTOR_FLAGS) ^ parent_flags| is non-zero to avoid doing extra work after the flags have already been set.

::: servo/components/style/stylist.rs
@@ +403,5 @@
>                                  CascadeFlags::empty());
>  
> +        // Apply the selector flags. We should be in sequential mode already,
> +        // so we can directly apply the parent flags.
> +        debug_assert!(thread_state::get().is_layout());

This is not true for servo I fear, servo may be in parallel mode here :(.

Though there's no point in doing this at all, you should be able to assert that flags.is_empty(), since this is for *|*::pseudo style selectors anyway.

@@ +405,5 @@
> +        // Apply the selector flags. We should be in sequential mode already,
> +        // so we can directly apply the parent flags.
> +        debug_assert!(thread_state::get().is_layout());
> +        let self_flags = flags.for_self();
> +        if self_flags.is_empty() {

I think I'm really missing something or these checks are really reversed?

::: third_party/rust/selectors/src/matching.rs
@@ +65,5 @@
>  }
>  
>  bitflags! {
> +    /// Set of flags that are set on either the element or its parent (depending
> +    /// on the flag) if the element could potentially match a selector.

Probably the name should highlight better that this is for when it could match a selector. This is pre-existing though, and I don't have an immediately better name for it though... shrug.

@@ +104,4 @@
>  pub fn matches<E>(selector_list: &[Selector<E::Impl>],
>                    element: &E,
>                    parent_bf: Option<&BloomFilter>,
> +                  flags: &mut ElementSelectorFlags)

You might want just to remove the parameter and ignore this, the same way we do for StyleRelations. But I don't feel strongly about it either, so your preference.

@@ +203,5 @@
>                        -> Option<SelectorMatchingResult>
>      where E: Element
>  {
>      if !selector.compound_selector.iter().all(|simple_selector| {
> +      matches_simple_selector(simple_selector, element, parent_bf, relations, flags) }) {

This is going to need fixing, please file an issue to refactor this so we don't match anything before rejecting with the bloom filter.

Also, we should do the bloom filter check once per selector, since it looks at all the chain, so the result is always the same, and right now this isn't true. Please file bugs for this if you have the time, otherwise remind me to fill them (need to run to class now).

Comment 16

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15) 
> The approach looks good, though I would want to take another look at this
> after the replies to my comments.

Yeah, I was bit rushed when putting the patch together last night. Sorry for any sloppy bits.
 
> ::: servo/components/script/dom/element.rs
> @@ +130,5 @@
> >      attr_list: MutNullableJS<NamedNodeMap>,
> >      class_list: MutNullableJS<DOMTokenList>,
> >      state: Cell<ElementState>,
> > +    #[ignore_heap_size_of = "bitflags defined in rust-selectors"]
> > +    selector_flags: Cell<ElementSelectorFlags>,
> 
> Maybe describe how these are set and why they're not atomic? It might not be
> obvious for someone working in the DOM.

Done.

> 
> @@ +350,5 @@
> >      fn namespace(&self) -> &Namespace;
> >      fn get_checked_state_for_layout(&self) -> bool;
> >      fn get_indeterminate_state_for_layout(&self) -> bool;
> >      fn get_state_for_layout(&self) -> ElementState;
> > +    fn insert_selector_flags(&self, flags: ElementSelectorFlags);
> 
> You probably also want a way to get the selector flags so you can avoid the
> churn of inserting them if it has already been done.

I'm not sure what sort of churn you're worried about. Setting already-set flags is idempotent, and faster than first testing and then setting. Remember how you wanted to call set_has_dirty_descendants() unconditionally? ;-)

> 
> @@ +719,5 @@
> >      }
> >  
> >      #[inline]
> >      #[allow(unsafe_code)]
> > +    fn insert_selector_flags(&self, flags: ElementSelectorFlags) {
> 
> I think you can assert here that thread_state::get() == LAYOUT, (note the
> equals, not the contains(), which would include layout workers).

Done.

> 
> @@ +1975,4 @@
> >          match SelectorParser::parse_author_origin_no_namespace(&selectors) {
> >              Err(()) => Err(Error::Syntax),
> >              Ok(selectors) => {
> > +                Ok(matches(&selectors.0, &Root::from_ref(self), None, &mut _flags))
> 
> What we do for the relations is just &mut ElementSelectorFlags::empty(),
> which is kind of self-explanatory. Does that not work for some reason?

Oh nice. I didn't realize you could pass a mutable borrow of a temporary. I've fixed all the callsites.

> ::: servo/components/style/context.rs
> @@ +179,5 @@
> > +impl<E: TElement> SequentialTask<E> {
> > +    /// Executes this task.
> > +    pub fn execute(self) {
> > +        use self::SequentialTask::*;
> > +        debug_assert!(thread_state::get().is_layout());
> 
> debug_assert!(!thread_state::get().is_worker());

Ah. That's a bit of a footgun. I've fixed it to do == comparison, like you suggested for the dom change.
> ::: servo/components/style/dom.rs
> @@ +324,5 @@
> >      fn skip_root_and_item_based_display_fixup(&self) -> bool;
> > +
> > +    /// Sets selector flags, which indicate what kinds of selectors may have
> > +    /// matched on this element and therefore what kind of work may need to
> > +    /// be performed when DOM state changes.
> 
> Note how they're set here too, I think it's important if you wonder why this
> method is marked unsafe.

I added a comment.

> 
> ::: servo/components/style/matching.rs
> @@ +34,5 @@
> >  #[inline]
> >  fn relations_are_shareable(relations: &StyleRelations) -> bool {
> >      use selectors::matching::*;
> >      !relations.intersects(AFFECTED_BY_ID_SELECTOR |
> > +                          AFFECTED_BY_CHILD_INDEX |
> 
> if this intentional? If so, why? and why is it right? Given we could still
> cache styles that wouldn't match child index selectors but could?

It was a drive-by fix yeah, and I can remove it for now. Given that StyleRelations only deals with selectors that matched, I agree that the fix is incomplete. But how does the style sharing cache deal with positional selectors? Do we need to file a bug?


> 
> @@ +631,5 @@
> >          }
> >  
> > +        // Apply the selector flags.
> > +        let self_flags = flags.for_self();
> > +        if self_flags.is_empty() {
> 
> You mean if !self_flags.is_empty(), right?

Yeah, had it backwards.

> 
> @@ +635,5 @@
> > +        if self_flags.is_empty() {
> > +            unsafe { self.set_selector_flags(self_flags); }
> > +        }
> > +        let parent_flags = flags.for_parent();
> > +        if parent_flags.is_empty() {
> 
> if !parent_flags.is_empty()?

Yeah same. It was copy-paste.

 Actually, I believe you want to check that
> |(p.get_flags() & ALL_SELECTOR_FLAGS) ^ parent_flags| is non-zero to avoid
> doing extra work after the flags have already been set.

Per above, I'm not sure why that saves us any work? It's true that in the Gecko case setting flags requires an FFI all whereas getting flags doesn't. But if flag setting ends up being any overhead, we should just fix inline it with bindgen.

> 
> ::: servo/components/style/stylist.rs
> @@ +403,5 @@
> >                                  CascadeFlags::empty());
> >  
> > +        // Apply the selector flags. We should be in sequential mode already,
> > +        // so we can directly apply the parent flags.
> > +        debug_assert!(thread_state::get().is_layout());
> 
> This is not true for servo I fear, servo may be in parallel mode here :(.
> 
> Though there's no point in doing this at all, you should be able to assert
> that flags.is_empty(), since this is for *|*::pseudo style selectors anyway.

Per IRC discussion this isn't true. But we found a solution.

> 
> @@ +405,5 @@
> > +        // Apply the selector flags. We should be in sequential mode already,
> > +        // so we can directly apply the parent flags.
> > +        debug_assert!(thread_state::get().is_layout());
> > +        let self_flags = flags.for_self();
> > +        if self_flags.is_empty() {
> 
> I think I'm really missing something or these checks are really reversed?

Yeah, fixed.

> 
> ::: third_party/rust/selectors/src/matching.rs
> @@ +65,5 @@
> >  }
> >  
> >  bitflags! {
> > +    /// Set of flags that are set on either the element or its parent (depending
> > +    /// on the flag) if the element could potentially match a selector.
> 
> Probably the name should highlight better that this is for when it could
> match a selector. This is pre-existing though, and I don't have an
> immediately better name for it though... shrug.
> 
> @@ +104,4 @@
> >  pub fn matches<E>(selector_list: &[Selector<E::Impl>],
> >                    element: &E,
> >                    parent_bf: Option<&BloomFilter>,
> > +                  flags: &mut ElementSelectorFlags)
> 
> You might want just to remove the parameter and ignore this, the same way we
> do for StyleRelations. But I don't feel strongly about it either, so your
> preference.
> 

That's a nice idea.

> @@ +203,5 @@
> >                        -> Option<SelectorMatchingResult>
> >      where E: Element
> >  {
> >      if !selector.compound_selector.iter().all(|simple_selector| {
> > +      matches_simple_selector(simple_selector, element, parent_bf, relations, flags) }) {
> 
> This is going to need fixing, please file an issue to refactor this so we
> don't match anything before rejecting with the bloom filter.
> 
> Also, we should do the bloom filter check once per selector, since it looks
> at all the chain, so the result is always the same, and right now this isn't
> true. Please file bugs for this if you have the time, otherwise remind me to
> fill them (need to run to class now).

Please file if you don't mind, I'm not super familiar with the code in question.

Comment 17

[Bobby Holley (:bholley)]

NI for the question on AFFECTED_BY_CHILD_INDEX above, just to be sure it doesn't get dropped.

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 3 - Apply selector flags during traversal. v3

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16)
> I'm not sure what sort of churn you're worried about. Setting already-set
> flags is idempotent, and faster than first testing and then setting.
> Remember how you wanted to call set_has_dirty_descendants() unconditionally?
> ;-)

Sure, I meant in the case we actually need to push a sequential task, you might as well skip all that work, I guess.
 
> It was a drive-by fix yeah, and I can remove it for now. Given that
> StyleRelations only deals with selectors that matched, I agree that the fix
> is incomplete. But how does the style sharing cache deal with positional
> selectors? Do we need to file a bug?

We have a list of sibling-affecting rules, where child index selectors are taken into account[1], and we test those. So nope :)

[1]: https://github.com/servo/rust-selectors/blob/master/src/parser.rs#L145

> Per above, I'm not sure why that saves us any work? It's true that in the
> Gecko case setting flags requires an FFI all whereas getting flags doesn't.
> But if flag setting ends up being any overhead, we should just fix inline it
> with bindgen.

It was about the task thing, not about the ffi overhead :)
 
> > This is not true for servo I fear, servo may be in parallel mode here :(.
> > 
> > Though there's no point in doing this at all, you should be able to assert
> > that flags.is_empty(), since this is for *|*::pseudo style selectors anyway.
> 
> Per IRC discussion this isn't true. But we found a solution.

If somebody wonders, the solution is that in servo lazy selectors (not precomputed, which were the ones I was trying to reference in my original comment) can't be public anyway, so we should be able to assert that anyway, because the UA sheet shouldn't have that nasty stuff.

> Please file if you don't mind, I'm not super familiar with the code in
> question.

Will do.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8834552 [details] [diff] [review]
Part 3 - Apply selector flags during traversal. v3

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

perfect, with that nit, ideally the extra trait instead of unsafe_get, and the suggestion from my reply to avoid adding unnecessary tasks, this is r=me :)

::: servo/components/script/dom/node.rs
@@ +322,5 @@
>          // (instead of passing `None`)? Probably.
>          self.iterator.by_ref().filter_map(|node| {
>              if let Some(element) = Root::downcast(node) {
> +                if matches(selectors, &element, None)
> +                {

nit: brace in the same line.

::: servo/components/script_layout_interface/wrapper_traits.rs
@@ +319,5 @@
> +    ///
> +    /// We need this so that the functions defined on this trait can call
> +    /// lazily_compute_pseudo_element_style, which operates on TElement.
> +    unsafe fn unsafe_get(self) ->
> +        <<Self::ConcreteThreadSafeLayoutNode as ThreadSafeLayoutNode>::ConcreteNode as TNode>::ConcreteElement;

We could also do the same thing I did for preshints, see the PresentationalHintSythetizer trait. May do as a followup if you want, but please at least file an issue in servo/sevo.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #21)
> nit: brace in the same line.

Fixed.

> 
> ::: servo/components/script_layout_interface/wrapper_traits.rs
> @@ +319,5 @@
> > +    ///
> > +    /// We need this so that the functions defined on this trait can call
> > +    /// lazily_compute_pseudo_element_style, which operates on TElement.
> > +    unsafe fn unsafe_get(self) ->
> > +        <<Self::ConcreteThreadSafeLayoutNode as ThreadSafeLayoutNode>::ConcreteNode as TNode>::ConcreteElement;
> 
> We could also do the same thing I did for preshints, see the
> PresentationalHintSythetizer trait. May do as a followup if you want, but
> please at least file an issue in servo/sevo.

I'm not sure what this would look like. Using a less-specific trait would look pretty much exactly like the ElementExt situation we have now. The problem with this is that it forces all of codepaths in stylist to operate on ElementExt rather than TElement, which is exactly what I'm trying to solve here.

Comment 23

[Bobby Holley (:bholley)]

Green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4468f8f1c528487bf66379a2c82188c908730219

Comment 24

[Bobby Holley (:bholley)]

bz says we never unset the flags, and this landed on the servo side. All done here.