From a2a4ec6ffb8051e522dd9a1a3c5f4f7b958c713e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 18 Jun 2023 13:06:43 +0200 Subject: [PATCH] style: Simplify selector flag setting now that flag setting is atomic These bits are write-only, actually, and we don't even need to read them. Differential Revision: https://phabricator.services.mozilla.com/D141888 --- components/style/context.rs | 77 +----------------------------- components/style/dom.rs | 26 +++++++--- components/style/gecko/wrapper.rs | 7 +-- components/style/matching.rs | 48 +------------------ components/style/sharing/checks.rs | 5 +- components/style/sharing/mod.rs | 13 +---- components/style/style_resolver.rs | 8 +--- components/style/stylist.rs | 19 +------- 8 files changed, 29 insertions(+), 174 deletions(-) diff --git a/components/style/context.rs b/components/style/context.rs index a7d5aa4a67b..633479c5a7a 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -27,8 +27,8 @@ use crate::traversal_flags::TraversalFlags; use app_units::Au; use euclid::default::Size2D; use euclid::Scale; +#[cfg(feature = "servo")] use fxhash::FxHashMap; -use selectors::matching::ElementSelectorFlags; use selectors::NthIndexCache; #[cfg(feature = "gecko")] use servo_arc::Arc; @@ -41,7 +41,6 @@ use style_traits::DevicePixel; #[cfg(feature = "servo")] use style_traits::SpeculativePainter; use time; -use uluru::LRUCache; pub use selectors::matching::QuirksMode; @@ -517,65 +516,6 @@ impl SequentialTask { } } -type CacheItem = (SendElement, ElementSelectorFlags); - -/// Map from Elements to ElementSelectorFlags. Used to defer applying selector -/// flags until after the traversal. -pub struct SelectorFlagsMap { - /// The hashmap storing the flags to apply. - map: FxHashMap, ElementSelectorFlags>, - /// An LRU cache to avoid hashmap lookups, which can be slow if the map - /// gets big. - cache: LRUCache, { 4 + 1 }>, -} - -#[cfg(debug_assertions)] -impl Drop for SelectorFlagsMap { - fn drop(&mut self) { - debug_assert!(self.map.is_empty()); - } -} - -impl SelectorFlagsMap { - /// Creates a new empty SelectorFlagsMap. - pub fn new() -> Self { - SelectorFlagsMap { - map: FxHashMap::default(), - cache: LRUCache::default(), - } - } - - /// Inserts some flags into the map for a given element. - pub fn insert_flags(&mut self, element: E, flags: ElementSelectorFlags) { - let el = unsafe { SendElement::new(element) }; - // Check the cache. If the flags have already been noted, we're done. - if let Some(item) = self.cache.find(|x| x.0 == el) { - if !item.1.contains(flags) { - item.1.insert(flags); - self.map.get_mut(&el).unwrap().insert(flags); - } - return; - } - - let f = self.map.entry(el).or_insert(ElementSelectorFlags::empty()); - *f |= flags; - - self.cache - .insert((unsafe { SendElement::new(element) }, *f)) - } - - /// Applies the flags. Must be called on the main thread. - fn apply_flags(&mut self) { - debug_assert_eq!(thread_state::get(), ThreadState::LAYOUT); - self.cache.clear(); - for (el, flags) in self.map.drain() { - unsafe { - el.set_selector_flags(flags); - } - } - } -} - /// A list of SequentialTasks that get executed on Drop. pub struct SequentialTaskList(Vec>) where @@ -697,11 +637,6 @@ pub struct ThreadLocalStyleContext { /// filter, to ensure they're dropped before we execute the tasks, which /// could create another ThreadLocalStyleContext for style computation. pub tasks: SequentialTaskList, - /// ElementSelectorFlags that need to be applied after the traversal is - /// complete. This map is used in cases where the matching algorithm needs - /// to set flags on elements it doesn't have exclusive access to (i.e. other - /// than the current element). - pub selector_flags: SelectorFlagsMap, /// Statistics about the traversal. pub statistics: PerThreadTraversalStatistics, /// A checker used to ensure that parallel.rs does not recurse indefinitely @@ -719,7 +654,6 @@ impl ThreadLocalStyleContext { rule_cache: RuleCache::new(), bloom_filter: StyleBloom::new(), tasks: SequentialTaskList(Vec::new()), - selector_flags: SelectorFlagsMap::new(), statistics: PerThreadTraversalStatistics::default(), stack_limit_checker: StackLimitChecker::new( (STYLE_THREAD_STACK_SIZE_KB - STACK_SAFETY_MARGIN_KB) * 1024, @@ -729,15 +663,6 @@ impl ThreadLocalStyleContext { } } -impl Drop for ThreadLocalStyleContext { - fn drop(&mut self) { - debug_assert_eq!(thread_state::get(), ThreadState::LAYOUT); - - // Apply any slow selector flags that need to be set on parents. - self.selector_flags.apply_flags(); - } -} - /// A `StyleContext` is just a simple container for a immutable reference to a /// shared style context, and a mutable reference to a local one. pub struct StyleContext<'a, E: TElement + 'a> { diff --git a/components/style/dom.rs b/components/style/dom.rs index 308006efd11..4148db8e8b5 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -738,14 +738,26 @@ pub trait TElement: /// matched on this element and therefore what kind of work may need to /// be performed when DOM state changes. /// - /// This is unsafe, like all the flag-setting methods, because it's only safe - /// to call with exclusive access to the element. When setting flags on the - /// parent during parallel traversal, we use SequentialTask to queue up the - /// set to run after the threads join. - unsafe fn set_selector_flags(&self, flags: ElementSelectorFlags); + /// You probably don't want to use this directly and want to use + /// apply_selector_flags, since that sets flags on the parent as needed. + fn set_selector_flags(&self, flags: ElementSelectorFlags); - /// Returns true if the element has all the specified selector flags. - fn has_selector_flags(&self, flags: ElementSelectorFlags) -> bool; + /// Applies selector flags to an element and its parent as needed. + fn apply_selector_flags(&self, flags: ElementSelectorFlags) { + // Handle flags that apply to the element. + let self_flags = flags.for_self(); + if !self_flags.is_empty() { + self.set_selector_flags(self_flags); + } + + // Handle flags that apply to the parent. + let parent_flags = flags.for_parent(); + if !parent_flags.is_empty() { + if let Some(p) = self.parent_element() { + p.set_selector_flags(parent_flags); + } + } + } /// In Gecko, element has a flag that represents the element may have /// any type of animations or not to bail out animation stuff early. diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index ca2c3816ec9..1da116faf38 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1393,16 +1393,11 @@ impl<'le> TElement for GeckoElement<'le> { self.is_root_of_native_anonymous_subtree() } - unsafe fn set_selector_flags(&self, flags: ElementSelectorFlags) { + fn set_selector_flags(&self, flags: ElementSelectorFlags) { debug_assert!(!flags.is_empty()); self.set_flags(selector_flags_to_node_flags(flags)); } - fn has_selector_flags(&self, flags: ElementSelectorFlags) -> bool { - let node_flags = selector_flags_to_node_flags(flags); - (self.flags() & node_flags) == node_flags - } - #[inline] fn may_have_animations(&self) -> bool { if let Some(pseudo) = self.implemented_pseudo_element() { diff --git a/components/style/matching.rs b/components/style/matching.rs index f2d300df4b2..58b8a10221f 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -8,7 +8,7 @@ #![deny(missing_docs)] use crate::computed_value_flags::ComputedValueFlags; -use crate::context::{CascadeInputs, ElementCascadeInputs, QuirksMode, SelectorFlagsMap}; +use crate::context::{CascadeInputs, ElementCascadeInputs, QuirksMode}; use crate::context::{SharedStyleContext, StyleContext}; use crate::data::{ElementData, ElementStyles}; use crate::dom::TElement; @@ -26,7 +26,6 @@ use crate::style_resolver::{PseudoElementResolution, StyleResolverForElement}; use crate::stylesheets::layer_rule::LayerOrder; use crate::stylist::RuleInclusion; use crate::traversal_flags::TraversalFlags; -use selectors::matching::ElementSelectorFlags; use servo_arc::{Arc, ArcBorrow}; /// Represents the result of comparing an element's old and new style. @@ -1007,51 +1006,6 @@ pub trait MatchMethods: TElement { cascade_requirement } - /// Applies selector flags to an element, deferring mutations of the parent - /// until after the traversal. - /// - /// TODO(emilio): This is somewhat inefficient, because it doesn't take - /// advantage of us knowing that the traversal is sequential. - fn apply_selector_flags( - &self, - map: &mut SelectorFlagsMap, - element: &Self, - flags: ElementSelectorFlags, - ) { - // Handle flags that apply to the element. - let self_flags = flags.for_self(); - if !self_flags.is_empty() { - if element == self { - // If this is the element we're styling, we have exclusive - // access to the element, and thus it's fine inserting them, - // even from the worker. - unsafe { - element.set_selector_flags(self_flags); - } - } else { - // Otherwise, this element is an ancestor of the current element - // we're styling, and thus multiple children could write to it - // if we did from here. - // - // Instead, we can read them, and post them if necessary as a - // sequential task in order for them to be processed later. - if !element.has_selector_flags(self_flags) { - map.insert_flags(*element, self_flags); - } - } - } - - // Handle flags that apply to the parent. - let parent_flags = flags.for_parent(); - if !parent_flags.is_empty() { - if let Some(p) = element.parent_element() { - if !p.has_selector_flags(parent_flags) { - map.insert_flags(p, parent_flags); - } - } - } - } - /// Updates the rule nodes without re-running selector matching, using just /// the rule tree. /// diff --git a/components/style/sharing/checks.rs b/components/style/sharing/checks.rs index 5e8350e78d3..2f8f410190f 100644 --- a/components/style/sharing/checks.rs +++ b/components/style/sharing/checks.rs @@ -7,7 +7,7 @@ //! elements can indeed share the same style. use crate::bloom::StyleBloom; -use crate::context::{SelectorFlagsMap, SharedStyleContext}; +use crate::context::SharedStyleContext; use crate::dom::TElement; use crate::sharing::{StyleSharingCandidate, StyleSharingTarget}; use selectors::NthIndexCache; @@ -120,7 +120,6 @@ pub fn revalidate( shared_context: &SharedStyleContext, bloom: &StyleBloom, nth_index_cache: &mut NthIndexCache, - selector_flags_map: &mut SelectorFlagsMap, ) -> bool where E: TElement, @@ -128,7 +127,7 @@ where let stylist = &shared_context.stylist; let for_element = - target.revalidation_match_results(stylist, bloom, nth_index_cache, selector_flags_map); + target.revalidation_match_results(stylist, bloom, nth_index_cache); let for_candidate = candidate.revalidation_match_results(stylist, bloom, nth_index_cache); diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 50eb51fba35..749504972a7 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -66,9 +66,8 @@ use crate::applicable_declarations::ApplicableDeclarationBlock; use crate::bloom::StyleBloom; -use crate::context::{SelectorFlagsMap, SharedStyleContext, StyleContext}; +use crate::context::{SharedStyleContext, StyleContext}; use crate::dom::{SendElement, TElement}; -use crate::matching::MatchMethods; use crate::properties::ComputedValues; use crate::rule_tree::StrongRuleNode; use crate::style_resolver::{PrimaryStyle, ResolvedElementStyles}; @@ -384,7 +383,6 @@ impl StyleSharingTarget { stylist: &Stylist, bloom: &StyleBloom, nth_index_cache: &mut NthIndexCache, - selector_flags_map: &mut SelectorFlagsMap, ) -> &SmallBitVec { // It's important to set the selector flags. Otherwise, if we succeed in // sharing the style, we may not set the slow selector flags for the @@ -401,9 +399,8 @@ impl StyleSharingTarget { // The style sharing cache will get a hit for the second span. When the // child span is subsequently removed from the DOM, missing selector // flags would cause us to miss the restyle on the second span. - let element = self.element; let mut set_selector_flags = |el: &E, flags: ElementSelectorFlags| { - element.apply_selector_flags(selector_flags_map, el, flags); + el.apply_selector_flags(flags); }; self.validation_data.revalidation_match_results( @@ -423,7 +420,6 @@ impl StyleSharingTarget { ) -> Option { let cache = &mut context.thread_local.sharing_cache; let shared_context = &context.shared; - let selector_flags_map = &mut context.thread_local.selector_flags; let bloom_filter = &context.thread_local.bloom_filter; let nth_index_cache = &mut context.thread_local.nth_index_cache; @@ -443,7 +439,6 @@ impl StyleSharingTarget { cache.share_style_if_possible( shared_context, - selector_flags_map, bloom_filter, nth_index_cache, self, @@ -667,7 +662,6 @@ impl StyleSharingCache { fn share_style_if_possible( &mut self, shared_context: &SharedStyleContext, - selector_flags_map: &mut SelectorFlagsMap, bloom_filter: &StyleBloom, nth_index_cache: &mut NthIndexCache, target: &mut StyleSharingTarget, @@ -700,7 +694,6 @@ impl StyleSharingCache { &shared_context, bloom_filter, nth_index_cache, - selector_flags_map, shared_context, ) }) @@ -712,7 +705,6 @@ impl StyleSharingCache { shared: &SharedStyleContext, bloom: &StyleBloom, nth_index_cache: &mut NthIndexCache, - selector_flags_map: &mut SelectorFlagsMap, shared_context: &SharedStyleContext, ) -> Option { debug_assert!(!target.is_in_native_anonymous_subtree()); @@ -817,7 +809,6 @@ impl StyleSharingCache { shared, bloom, nth_index_cache, - selector_flags_map, ) { trace!("Miss: Revalidation"); return None; diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index 81517f6cd64..d829f3f8e86 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -451,7 +451,6 @@ where ); let mut applicable_declarations = ApplicableDeclarationList::new(); - let map = &mut self.context.thread_local.selector_flags; let bloom_filter = self.context.thread_local.bloom_filter.filter(); let nth_index_cache = &mut self.context.thread_local.nth_index_cache; let mut matching_context = MatchingContext::new_for_visited( @@ -465,9 +464,8 @@ where let stylist = &self.context.shared.stylist; let implemented_pseudo = self.element.implemented_pseudo_element(); { - let resolving_element = self.element; let mut set_selector_flags = |element: &E, flags: ElementSelectorFlags| { - resolving_element.apply_selector_flags(map, element, flags); + element.apply_selector_flags(flags); }; // Compute the primary rule node. @@ -542,10 +540,8 @@ where self.context.shared.quirks_mode(), ); - let map = &mut self.context.thread_local.selector_flags; - let resolving_element = self.element; let mut set_selector_flags = |element: &E, flags: ElementSelectorFlags| { - resolving_element.apply_selector_flags(map, element, flags); + element.apply_selector_flags(flags); }; // NB: We handle animation rules for ::before and ::after when diff --git a/components/style/stylist.rs b/components/style/stylist.rs index a1ad4aab659..4c19a41bcd6 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -39,7 +39,6 @@ use crate::stylesheets::{ CssRule, EffectiveRulesIterator, Origin, OriginSet, PageRule, PerOrigin, PerOriginIter, }; use crate::stylesheets::{StyleRule, StylesheetContents, StylesheetInDocument}; -use crate::thread_state::{self, ThreadState}; use crate::AllocErr; use crate::{Atom, LocalName, Namespace, ShrinkIfNeeded, WeakAtom}; use fxhash::FxHashMap; @@ -1088,23 +1087,7 @@ impl Stylist { return; } - // Gecko calls this from sequential mode, so we can directly apply - // the flags. - debug_assert_eq!(thread_state::get(), ThreadState::LAYOUT); - let self_flags = flags.for_self(); - if !self_flags.is_empty() { - unsafe { - element.set_selector_flags(self_flags); - } - } - let parent_flags = flags.for_parent(); - if !parent_flags.is_empty() { - if let Some(p) = element.parent_element() { - unsafe { - p.set_selector_flags(parent_flags); - } - } - } + element.apply_selector_flags(flags); }; let mut declarations = ApplicableDeclarationList::new();