diff --git a/layout/style/ServoElementSnapshot.cpp b/layout/style/ServoElementSnapshot.cpp index 5aaf6f7cf3d7..99b0ae6c861b 100644 --- a/layout/style/ServoElementSnapshot.cpp +++ b/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()); diff --git a/layout/style/ServoElementSnapshot.h b/layout/style/ServoElementSnapshot.h index 0469358c69e4..52069e280ead 100644 --- a/layout/style/ServoElementSnapshot.h +++ b/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 mAttrs; + nsTArray> 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()) { diff --git a/servo/components/style/gecko/selector_parser.rs b/servo/components/style/gecko/selector_parser.rs index a46068d923a4..e5d332df6896 100644 --- a/servo/components/style/gecko/selector_parser.rs +++ b/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 { diff --git a/servo/components/style/gecko/snapshot.rs b/servo/components/style/gecko/snapshot.rs index b2a66f709e25..26b3e1393dd0 100644 --- a/servo/components/style/gecko/snapshot.rs +++ b/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(&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 diff --git a/servo/components/style/invalidation/element/invalidation_map.rs b/servo/components/style/invalidation/element/invalidation_map.rs index 8bc0f07a2bf0..f6c81a0ae02c 100644 --- a/servo/components/style/invalidation/element/invalidation_map.rs +++ b/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, /// A map of other attribute affecting selectors. - pub other_attribute_affecting_selectors: SelectorMap, - /// 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>, } 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 } } diff --git a/servo/components/style/invalidation/element/state_and_attributes.rs b/servo/components/style/invalidation/element/state_and_attributes.rs index 828bc99ed20b..25536eb6bdfd 100644 --- a/servo/components/style/invalidation/element/state_and_attributes.rs +++ b/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) { - 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, diff --git a/servo/components/style/servo/selector_parser.rs b/servo/components/style/servo/selector_parser.rs index 7f8ac65089d4..d8b84b13280f 100644 --- a/servo/components/style/servo/selector_parser.rs +++ b/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