diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index ff60ecc32b9..055e3e6f3ac 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -35,6 +35,16 @@ pub struct TreeStyleInvalidator<'a, 'b: 'a, E> type InvalidationVector = SmallVec<[Invalidation; 10]>; +/// The kind of invalidation we're processing. +/// +/// We can use this to avoid pushing invalidations of the same kind to our +/// descendants or siblings. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +enum InvalidationKind { + Descendant, + Sibling, +} + /// An `Invalidation` is a complex selector that describes which elements, /// relative to a current element we are processing, must be restyled. /// @@ -46,6 +56,37 @@ type InvalidationVector = SmallVec<[Invalidation; 10]>; struct Invalidation { selector: Selector, offset: usize, + /// Whether the invalidation was already matched by any previous sibling or + /// ancestor. + /// + /// If this is the case, we can avoid pushing invalidations generated by + /// this one if the generated invalidation is effective for all the siblings + /// or descendants after us. + matched_by_any_previous: bool, +} + +impl Invalidation { + /// Whether this invalidation is effective for the next sibling or + /// descendant after us. + fn effective_for_next(&self) -> bool { + // TODO(emilio): For pseudo-elements this should be mostly false, except + // for the weird pseudos in . + // + // We should be able to do better here! + match self.selector.combinator_at(self.offset) { + Combinator::NextSibling | + Combinator::Child => false, + _ => true, + } + } + + fn kind(&self) -> InvalidationKind { + if self.selector.combinator_at(self.offset).is_ancestor() { + InvalidationKind::Descendant + } else { + InvalidationKind::Sibling + } + } } impl fmt::Debug for Invalidation { @@ -67,9 +108,9 @@ impl fmt::Debug for Invalidation { struct InvalidationResult { /// Whether the element itself was invalidated. invalidated_self: bool, - /// Whether the invalidation we've processed is effective for the next - /// sibling or descendant after us. - effective_for_next: bool, + /// Whether the invalidation matched, either invalidating the element or + /// generating another invalidation. + matched: bool, } impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> @@ -460,14 +501,16 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> let result = self.process_invalidation( &sibling_invalidations[i], descendant_invalidations, - &mut new_sibling_invalidations + &mut new_sibling_invalidations, + InvalidationKind::Sibling, ); invalidated_self |= result.invalidated_self; - if !result.effective_for_next { - sibling_invalidations.remove(i); - } else { + sibling_invalidations[i].matched_by_any_previous |= result.matched; + if sibling_invalidations[i].effective_for_next() { i += 1; + } else { + sibling_invalidations.remove(i); } } @@ -493,10 +536,13 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> invalidation, descendant_invalidations, sibling_invalidations, + InvalidationKind::Descendant, ); invalidated |= result.invalidated_self; - if result.effective_for_next { + if invalidation.effective_for_next() { + let mut invalidation = invalidation.clone(); + invalidation.matched_by_any_previous |= result.matched; descendant_invalidations.push(invalidation.clone()); } } @@ -514,10 +560,11 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> &mut self, invalidation: &Invalidation, descendant_invalidations: &mut InvalidationVector, - sibling_invalidations: &mut InvalidationVector + sibling_invalidations: &mut InvalidationVector, + invalidation_kind: InvalidationKind, ) -> InvalidationResult { - debug!("TreeStyleInvalidator::process_invalidation({:?}, {:?})", - self.element, invalidation); + debug!("TreeStyleInvalidator::process_invalidation({:?}, {:?}, {:?})", + self.element, invalidation, invalidation_kind); let mut context = MatchingContext::new_for_visited( @@ -535,14 +582,17 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> ); let mut invalidated_self = false; + let mut matched = false; match matching_result { CompoundSelectorMatchingResult::Matched { next_combinator_offset: 0 } => { debug!(" > Invalidation matched completely"); + matched = true; invalidated_self = true; } CompoundSelectorMatchingResult::Matched { next_combinator_offset } => { let next_combinator = invalidation.selector.combinator_at(next_combinator_offset); + matched = true; if matches!(next_combinator, Combinator::PseudoElement) { let pseudo_selector = @@ -578,14 +628,90 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> let next_invalidation = Invalidation { selector: invalidation.selector.clone(), offset: next_combinator_offset, + matched_by_any_previous: false, }; debug!(" > Invalidation matched, next: {:?}, ({:?})", next_invalidation, next_combinator); - if next_combinator.is_ancestor() { - descendant_invalidations.push(next_invalidation); + + let next_invalidation_kind = next_invalidation.kind(); + + // We can skip pushing under some circumstances, and we should + // because otherwise the invalidation list could grow + // exponentially. + // + // * First of all, both invalidations need to be of the same + // kind. This is because of how we propagate them going to + // the right of the tree for sibling invalidations and going + // down the tree for children invalidations. A sibling + // invalidation that ends up generating a children + // invalidation ends up (correctly) in five different lists, + // not in the same list five different times. + // + // * Then, the invalidation needs to be matched by a previous + // ancestor/sibling, in order to know that this invalidation + // has been generated already. + // + // * Finally, the new invalidation needs to be + // `effective_for_next()`, in order for us to know that it is + // still in the list, since we remove the dependencies that + // aren't from the lists for our children / siblings. + // + // To go through an example, let's imagine we are processing a + // dom subtree like: + // + //
+ // + // And an invalidation list with a single invalidation like: + // + // [div div div] + // + // When we process the invalidation list for the outer div, we + // match it, and generate a `div div` invalidation, so for the + //
child we have: + // + // [div div div, div div] + // + // With the first of them marked as `matched`. + // + // When we process the
child, we don't match any of + // them, so both invalidations go untouched to our children. + // + // When we process the second
, we match _both_ + // invalidations. + // + // However, when matching the first, we can tell it's been + // matched, and not push the corresponding `div div` + // invalidation, since we know it's necessarily already on the + // list. + // + // Thus, without skipping the push, we'll arrive to the + // innermost
with: + // + // [div div div, div div, div div, div] + // + // While skipping it, we won't arrive here with duplicating + // dependencies: + // + // [div div div, div div, div] + // + let can_skip_pushing = + next_invalidation_kind == invalidation_kind && + invalidation.matched_by_any_previous && + next_invalidation.effective_for_next(); + + if can_skip_pushing { + debug!(" > Can avoid push, since the invalidation had \ + already been matched before"); } else { - sibling_invalidations.push(next_invalidation); + match next_invalidation_kind { + InvalidationKind::Descendant => { + descendant_invalidations.push(next_invalidation); + } + InvalidationKind::Sibling => { + sibling_invalidations.push(next_invalidation); + } + } } } CompoundSelectorMatchingResult::NotMatched => {} @@ -597,21 +723,7 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> } } - // TODO(emilio): For pseudo-elements this should be mostly false, except - // for the weird pseudos in . - // - // We should be able to do better here! - let effective_for_next = - match invalidation.selector.combinator_at(invalidation.offset) { - Combinator::NextSibling | - Combinator::Child => false, - _ => true, - }; - - InvalidationResult { - invalidated_self: invalidated_self, - effective_for_next: effective_for_next, - } + InvalidationResult { invalidated_self, matched, } } } @@ -830,12 +942,14 @@ impl<'a, 'b: 'a, E> InvalidationCollector<'a, 'b, E> self.descendant_invalidations.push(Invalidation { selector: dependency.selector.clone(), offset: dependency.selector_offset, + matched_by_any_previous: false, }); } else if dependency.affects_later_siblings() { debug_assert_ne!(dependency.selector_offset, 0); self.sibling_invalidations.push(Invalidation { selector: dependency.selector.clone(), offset: dependency.selector_offset, + matched_by_any_previous: false, }); } }