Bug 1640843 - Finer grained invalidation for attribute changes. r=heycam

This should help out quite a bit with uBO, which has lots of very
general attribute selectors. We invalidate per attribute name rather
than using a SelectorMap, which prevents matching for attribute
selectors that can't have changed.

The idea is that this should be generally cheaper, though there are
cases where this would be a slight pesimization. For example, if there's
an attribute selector like:

  my-specific-element[my-attribute] { /* ... */ }

And you change `my-attribute` in an element that isn't a
`my-specific-element`, before that the SelectorMap would've prevented us
from selector-matching completely. Now we'd still run selector-matching
for that (though the matching would be pretty cheap).

However I think this should speed up things generally, let's see what
the perf tests think before landing this though.

Differential Revision: https://phabricator.services.mozilla.com/D76825

layout/style/ServoElementSnapshot.cpp

@@ -18,8 +18,7 @@ ServoElementSnapshot::ServoElementSnapshot(const Element& aElement)
       mIsTableBorderNonzero(false),
       mIsMozBrowserFrame(false),
       mClassAttributeChanged(false),
-      mIdAttributeChanged(false),
-      mOtherAttributeChanged(false) {
+      mIdAttributeChanged(false) {
   MOZ_COUNT_CTOR(ServoElementSnapshot);
   MOZ_ASSERT(NS_IsMainThread());
   mIsInChromeDocument = nsContentUtils::IsChromeDoc(aElement.OwnerDoc());

layout/style/ServoElementSnapshot.h

@@ -148,6 +148,7 @@ class ServoElementSnapshot {
   // though it can be wasted space if we deal with a lot of state-only
   // snapshots.
   nsTArray<AttrArray::InternalAttr> mAttrs;
+  nsTArray<RefPtr<nsAtom>> mChangedAttrNames;
   nsAttrValue mClass;
   ServoStateType mState;
   Flags mContains;
@@ -157,7 +158,6 @@ class ServoElementSnapshot {
   bool mIsMozBrowserFrame : 1;
   bool mClassAttributeChanged : 1;
   bool mIdAttributeChanged : 1;
-  bool mOtherAttributeChanged : 1;
 };
 
 inline void ServoElementSnapshot::AddAttrs(const Element& aElement,
@@ -165,14 +165,20 @@ inline void ServoElementSnapshot::AddAttrs(const Element& aElement,
                                            nsAtom* aAttribute) {
   if (aNameSpaceID == kNameSpaceID_None) {
     if (aAttribute == nsGkAtoms::_class) {
+      if (mClassAttributeChanged) {
+        return;
+      }
       mClassAttributeChanged = true;
     } else if (aAttribute == nsGkAtoms::id) {
+      if (mIdAttributeChanged) {
+        return;
+      }
       mIdAttributeChanged = true;
-    } else {
-      mOtherAttributeChanged = true;
     }
-  } else {
-    mOtherAttributeChanged = true;
+  }
+
+  if (!mChangedAttrNames.Contains(aAttribute)) {
+    mChangedAttrNames.AppendElement(aAttribute);
   }
 
   if (HasAttrs()) {

servo/components/style/gecko/selector_parser.rs

@@ -229,17 +229,6 @@ impl NonTSPseudoClass {
                       NonTSPseudoClass::MozLWThemeDarkText
             )
     }
-
-    /// Returns true if the evaluation of the pseudo-class depends on the
-    /// element's attributes.
-    pub fn is_attr_based(&self) -> bool {
-        matches!(
-            *self,
-            NonTSPseudoClass::MozTableBorderNonzero |
-                NonTSPseudoClass::MozBrowserFrame |
-                NonTSPseudoClass::Lang(..)
-        )
-    }
 }
 
 impl ::selectors::parser::NonTSPseudoClass for NonTSPseudoClass {

servo/components/style/gecko/snapshot.rs

@@ -70,11 +70,15 @@ impl GeckoElementSnapshot {
         self.mClassAttributeChanged()
     }
 
-    /// Returns true if the snapshot recorded an attribute change which isn't a
-    /// class / id
+    /// Executes the callback once for each attribute that changed.
     #[inline]
-    pub fn other_attr_changed(&self) -> bool {
-        self.mOtherAttributeChanged()
+    pub fn each_attr_changed<F>(&self, mut callback: F)
+    where
+        F: FnMut(&Atom),
+    {
+        for attr in self.mChangedAttrNames.iter() {
+            unsafe { Atom::with(attr.mRawPtr, &mut callback) }
+        }
     }
 
     /// selectors::Element::attr_matches

servo/components/style/invalidation/element/invalidation_map.rs

@@ -6,10 +6,10 @@
 
 use crate::context::QuirksMode;
 use crate::element_state::{DocumentState, ElementState};
-use crate::selector_map::{MaybeCaseInsensitiveHashMap, SelectorMap, SelectorMapEntry};
+use crate::selector_map::{MaybeCaseInsensitiveHashMap, PrecomputedHashMap, SelectorMap, SelectorMapEntry};
 use crate::selector_parser::SelectorImpl;
 use crate::{Atom, LocalName, Namespace};
-use fallible::FallibleVec;
+use fallible::{FallibleVec, FallibleHashMap};
 use hashbrown::CollectionAllocErr;
 use selectors::attr::NamespaceConstraint;
 use selectors::parser::{Combinator, Component};
@@ -175,19 +175,6 @@ pub struct DocumentStateDependency {
     pub state: DocumentState,
 }
 
-bitflags! {
-    /// A set of flags that denote whether any invalidations have occurred
-    /// for a particular attribute selector.
-    #[derive(MallocSizeOf)]
-    #[repr(C)]
-    pub struct InvalidationMapFlags : u8 {
-        /// Whether [class] or such is used.
-        const HAS_CLASS_ATTR_SELECTOR = 1 << 0;
-        /// Whether [id] or such is used.
-        const HAS_ID_ATTR_SELECTOR = 1 << 1;
-    }
-}
-
 /// A map where we store invalidations.
 ///
 /// This is slightly different to a SelectorMap, in the sense of that the same
@@ -209,10 +196,7 @@ pub struct InvalidationMap {
     /// A list of document state dependencies in the rules we represent.
     pub document_state_selectors: Vec<DocumentStateDependency>,
     /// A map of other attribute affecting selectors.
-    pub other_attribute_affecting_selectors: SelectorMap<Dependency>,
-    /// A set of flags that contain whether various special attributes are used
-    /// in this invalidation map.
-    pub flags: InvalidationMapFlags,
+    pub other_attribute_affecting_selectors: PrecomputedHashMap<Atom, SmallVec<[Dependency; 1]>>,
 }
 
 impl InvalidationMap {
@@ -223,8 +207,7 @@ impl InvalidationMap {
             id_to_selector: MaybeCaseInsensitiveHashMap::new(),
             state_affecting_selectors: SelectorMap::new(),
             document_state_selectors: Vec::new(),
-            other_attribute_affecting_selectors: SelectorMap::new(),
-            flags: InvalidationMapFlags::empty(),
+            other_attribute_affecting_selectors: PrecomputedHashMap::default(),
         }
     }
 
@@ -232,7 +215,9 @@ impl InvalidationMap {
     pub fn len(&self) -> usize {
         self.state_affecting_selectors.len() +
             self.document_state_selectors.len() +
-            self.other_attribute_affecting_selectors.len() +
+            self.other_attribute_affecting_selectors
+                .iter()
+                .fold(0, |accum, (_, ref v)| accum + v.len()) +
             self.id_to_selector
                 .iter()
                 .fold(0, |accum, (_, ref v)| accum + v.len()) +
@@ -248,7 +233,6 @@ impl InvalidationMap {
         self.state_affecting_selectors.clear();
         self.document_state_selectors.clear();
         self.other_attribute_affecting_selectors.clear();
-        self.flags = InvalidationMapFlags::empty();
     }
 
     /// Adds a selector to this `InvalidationMap`.  Returns Err(..) to
@@ -300,9 +284,6 @@ struct PerCompoundState {
 
     /// The state this compound selector is affected by.
     element_state: ElementState,
-
-    /// Whether we've added a generic attribute dependency for this selector.
-    added_attr_dependency: bool,
 }
 
 impl PerCompoundState {
@@ -310,7 +291,6 @@ impl PerCompoundState {
         Self {
             offset,
             element_state: ElementState::empty(),
-            added_attr_dependency: false,
         }
     }
 }
@@ -387,20 +367,25 @@ impl<'a> SelectorDependencyCollector<'a> {
         }
     }
 
-    fn add_attr_dependency(&mut self) -> bool {
-        debug_assert!(!self.compound_state.added_attr_dependency);
-        self.compound_state.added_attr_dependency = true;
-
+    fn add_attr_dependency(&mut self, name: LocalName) -> bool {
         let dependency = self.dependency();
-        let result = self.map.other_attribute_affecting_selectors.insert(
-            dependency,
-            self.quirks_mode,
-        );
-        if let Err(alloc_error) = result {
-            *self.alloc_error = Some(alloc_error);
-            return false;
+
+        let map = &mut self.map.other_attribute_affecting_selectors;
+        let entry = match map.try_entry(name) {
+            Ok(entry) => entry,
+            Err(err) => {
+                *self.alloc_error = Some(err);
+                return false;
+            },
+        };
+
+        match entry.or_insert_with(SmallVec::new).try_push(dependency) {
+            Ok(..) => true,
+            Err(err) => {
+                *self.alloc_error = Some(err);
+                return false;
+            }
         }
-        true
     }
 
     fn dependency(&self) -> Dependency {
@@ -494,7 +479,13 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> {
                         return false;
                     },
                 };
-                entry.or_insert_with(SmallVec::new).try_push(dependency).is_ok()
+                match entry.or_insert_with(SmallVec::new).try_push(dependency) {
+                    Ok(..) => true,
+                    Err(err) => {
+                        *self.alloc_error = Some(err);
+                        return false;
+                    }
+                }
             },
             Component::NonTSPseudoClass(ref pc) => {
                 self.compound_state.element_state |= match *pc {
@@ -504,11 +495,16 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> {
                 };
                 *self.document_state |= pc.document_state_flag();
 
-                if !self.compound_state.added_attr_dependency && pc.is_attr_based() {
-                    self.add_attr_dependency()
-                } else {
-                    true
-                }
+                let attr_name = match *pc {
+                    #[cfg(feature = "gecko")]
+                    NonTSPseudoClass::MozTableBorderNonzero => local_name!("border"),
+                    #[cfg(feature = "gecko")]
+                    NonTSPseudoClass::MozBrowserFrame => local_name!("mozbrowser"),
+                    NonTSPseudoClass::Lang(..) => local_name!("lang"),
+                    _ => return true,
+                };
+
+                self.add_attr_dependency(attr_name)
             },
             _ => true,
         }
@@ -516,27 +512,18 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> {
 
     fn visit_attribute_selector(
         &mut self,
-        constraint: &NamespaceConstraint<&Namespace>,
-        _local_name: &LocalName,
+        _: &NamespaceConstraint<&Namespace>,
+        local_name: &LocalName,
         local_name_lower: &LocalName,
     ) -> bool {
-        let may_match_in_no_namespace = match *constraint {
-            NamespaceConstraint::Any => true,
-            NamespaceConstraint::Specific(ref ns) => ns.is_empty(),
-        };
-
-        if may_match_in_no_namespace {
-            if *local_name_lower == local_name!("id") {
-                self.map.flags.insert(InvalidationMapFlags::HAS_ID_ATTR_SELECTOR)
-            } else if *local_name_lower == local_name!("class") {
-                self.map.flags.insert(InvalidationMapFlags::HAS_CLASS_ATTR_SELECTOR)
-            }
+        if !self.add_attr_dependency(local_name.clone()) {
+            return false;
         }
 
-        if !self.compound_state.added_attr_dependency {
-            self.add_attr_dependency()
-        } else {
-            true
+        if local_name != local_name_lower && !self.add_attr_dependency(local_name_lower.clone()) {
+            return false;
         }
+
+        true
     }
 }

servo/components/style/invalidation/element/state_and_attributes.rs

@@ -42,7 +42,6 @@ where
     descendant_invalidations: &'a mut DescendantInvalidationLists<'selectors>,
     sibling_invalidations: &'a mut InvalidationVector<'selectors>,
     invalidates_self: bool,
-    attr_selector_flags: InvalidationMapFlags,
 }
 
 /// An invalidation processor for style changes due to state and attribute
@@ -197,8 +196,6 @@ where
             return false;
         }
 
-        let mut attr_selector_flags = InvalidationMapFlags::empty();
-
         // If we the visited state changed, we force a restyle here. Matching
         // doesn't depend on the actual visited state at all, so we can't look
         // at matching results to decide what to do for this case.
@@ -216,7 +213,6 @@ where
         let mut classes_removed = SmallVec::<[Atom; 8]>::new();
         let mut classes_added = SmallVec::<[Atom; 8]>::new();
         if snapshot.class_changed() {
-            attr_selector_flags.insert(InvalidationMapFlags::HAS_CLASS_ATTR_SELECTOR);
             // TODO(emilio): Do this more efficiently!
             snapshot.each_class(|c| {
                 if !element.has_class(c, CaseSensitivity::CaseSensitive) {
@@ -234,7 +230,6 @@ where
         let mut id_removed = None;
         let mut id_added = None;
         if snapshot.id_changed() {
-            attr_selector_flags.insert(InvalidationMapFlags::HAS_ID_ATTR_SELECTOR);
             let old_id = snapshot.id_attr();
             let current_id = element.id();
 
@@ -245,10 +240,7 @@ where
         }
 
         if log_enabled!(::log::Level::Debug) {
-            debug!(
-                "Collecting changes for: {:?}, flags {:?}",
-                element, attr_selector_flags
-            );
+            debug!("Collecting changes for: {:?}", element);
             if !state_changes.is_empty() {
                 debug!(" > state: {:?}", state_changes);
             }
@@ -261,7 +253,9 @@ where
                     classes_added, classes_removed
                 );
             }
-            if snapshot.other_attr_changed() {
+            let mut attributes_changed = false;
+            snapshot.each_attr_changed(|_| { attributes_changed = true; });
+            if attributes_changed {
                 debug!(
                     " > attributes changed, old: {}",
                     snapshot.debug_list_attributes()
@@ -296,7 +290,6 @@ where
                 descendant_invalidations,
                 sibling_invalidations,
                 invalidates_self: false,
-                attr_selector_flags,
             };
 
             let document_origins = if !matches_document_author_rules {
@@ -406,12 +399,13 @@ where
             }
         }
 
-        let should_examine_attribute_selector_map =
-            self.snapshot.other_attr_changed() || map.flags.intersects(self.attr_selector_flags);
-
-        if should_examine_attribute_selector_map {
-            self.collect_dependencies_in_map(&map.other_attribute_affecting_selectors)
-        }
+        self.snapshot.each_attr_changed(|attribute| {
+            if let Some(deps) = map.other_attribute_affecting_selectors.get(attribute) {
+                for dep in deps {
+                    self.scan_dependency(dep);
+                }
+            }
+        });
 
         let state_changes = self.state_changes;
         if !state_changes.is_empty() {
@@ -419,19 +413,6 @@ where
         }
     }
 
-    fn collect_dependencies_in_map(&mut self, map: &'selectors SelectorMap<Dependency>) {
-        map.lookup_with_additional(
-            self.lookup_element,
-            self.matching_context.quirks_mode(),
-            self.removed_id,
-            self.classes_removed,
-            |dependency| {
-                self.scan_dependency(dependency);
-                true
-            },
-        );
-    }
-
     fn collect_state_dependencies(
         &mut self,
         map: &'selectors SelectorMap<StateDependency>,

servo/components/style/servo/selector_parser.rs

@@ -391,12 +391,6 @@ impl NonTSPseudoClass {
     pub fn needs_cache_revalidation(&self) -> bool {
         self.state_flag().is_empty()
     }
-
-    /// Returns true if the evaluation of the pseudo-class depends on the
-    /// element's attributes.
-    pub fn is_attr_based(&self) -> bool {
-        matches!(*self, NonTSPseudoClass::Lang(..))
-    }
 }
 
 /// The abstract struct we implement the selector parser implementation on top