From a98fd5e2b63d9dbb6beb815343c3a0550e603f59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 22 Aug 2017 14:41:48 +0200 Subject: [PATCH 01/10] style: Make the StyleSheetSet::flush API slightly nicer. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: LfJxZA9zhaE Signed-off-by: Emilio Cobos Álvarez --- components/style/stylesheet_set.rs | 11 +++++------ components/style/stylist.rs | 12 ++++++++---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index 8548ac53f01..0d8d032402f 100644 --- a/components/style/stylesheet_set.rs +++ b/components/style/stylesheet_set.rs @@ -180,14 +180,12 @@ where .any(|(d, _)| d.dirty) } - /// Flush the current set, unmarking it as dirty, and returns an iterator - /// over the new stylesheet list. - /// - /// Returns true if any elements were invalidated. + /// Flush the current set, unmarking it as dirty, and returns the damaged + /// origins, and whether any elements were invalidated. pub fn flush( &mut self, document_element: Option, - ) -> (StylesheetIterator, OriginSet, bool) + ) -> (OriginSet, bool) where E: TElement, { @@ -195,6 +193,7 @@ where let mut origins = OriginSet::empty(); let mut have_invalidations = false; + for (data, origin) in self.invalidation_data.iter_mut_origins() { if data.dirty { have_invalidations |= data.invalidations.flush(document_element); @@ -203,7 +202,7 @@ where } } - (self.iter(), origins, have_invalidations) + (origins, have_invalidations) } /// Flush stylesheets, but without running any of the invalidation passes. diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 2ea8d5313ac..38b0861c047 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -490,13 +490,15 @@ impl Stylist { } let author_style_disabled = self.stylesheets.author_style_disabled(); - let (doc_stylesheets, origins_to_rebuild, have_invalidations) = + let (origins_to_rebuild, have_invalidations) = self.stylesheets.flush(document_element); if origins_to_rebuild.is_empty() { return have_invalidations; } + let doc_stylesheets = self.stylesheets.iter(); + self.num_rebuilds += 1; // Update viewport_constraints regardless of which origins' @@ -520,9 +522,11 @@ impl Stylist { }; self.viewport_constraints = - ViewportConstraints::maybe_new(&self.device, - &cascaded_rule, - self.quirks_mode); + ViewportConstraints::maybe_new( + &self.device, + &cascaded_rule, + self.quirks_mode, + ); if let Some(ref constraints) = self.viewport_constraints { self.device.account_for_viewport_rule(constraints); From e1517d62af779b6760467a9ba91865bb8d2aff65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 22 Aug 2017 15:07:33 +0200 Subject: [PATCH 02/10] style: Simplify and improve the per origin stylesheet invalidation setup. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: adDDRhNnOw Signed-off-by: Emilio Cobos Álvarez --- components/style/stylesheet_set.rs | 86 +++++++------------------- components/style/stylesheets/origin.rs | 1 + 2 files changed, 25 insertions(+), 62 deletions(-) diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index 0d8d032402f..4158cf0b796 100644 --- a/components/style/stylesheet_set.rs +++ b/components/style/stylesheet_set.rs @@ -9,7 +9,7 @@ use invalidation::stylesheets::StylesheetInvalidationSet; use media_queries::Device; use shared_lock::SharedRwLockReadGuard; use std::slice; -use stylesheets::{OriginSet, PerOrigin, StylesheetInDocument}; +use stylesheets::{Origin, OriginSet, StylesheetInDocument}; /// Entry for a StylesheetSet. We don't bother creating a constructor, because /// there's no sensible defaults for the member variables. @@ -51,8 +51,11 @@ where /// include recursive `@import` rules. entries: Vec>, - /// Per-origin stylesheet invalidation data. - invalidation_data: PerOrigin, + /// The invalidations for stylesheets added or removed from this document. + invalidations: StylesheetInvalidationSet, + + /// The origins whose stylesheets have changed so far. + origins_dirty: OriginSet, /// Has author style been disabled? author_style_disabled: bool, @@ -66,7 +69,8 @@ where pub fn new() -> Self { StylesheetSet { entries: vec![], - invalidation_data: Default::default(), + invalidations: StylesheetInvalidationSet::new(), + origins_dirty: OriginSet::empty(), author_style_disabled: false, } } @@ -97,12 +101,10 @@ where sheet: &S, guard: &SharedRwLockReadGuard, ) { - let origin = sheet.contents(guard).origin; - let data = self.invalidation_data.borrow_mut_for_origin(&origin); if let Some(device) = device { - data.invalidations.collect_invalidations_for(device, sheet, guard); + self.invalidations.collect_invalidations_for(device, sheet, guard); } - data.dirty = true; + self.origins_dirty |= sheet.contents(guard).origin; } /// Appends a new stylesheet to the current set. @@ -169,15 +171,13 @@ where return; } self.author_style_disabled = disabled; - self.invalidation_data.author.invalidations.invalidate_fully(); - self.invalidation_data.author.dirty = true; + self.invalidations.invalidate_fully(); + self.origins_dirty |= Origin::Author; } /// Returns whether the given set has changed from the last flush. pub fn has_changed(&self) -> bool { - self.invalidation_data - .iter_origins() - .any(|(d, _)| d.dirty) + !self.origins_dirty.is_empty() } /// Flush the current set, unmarking it as dirty, and returns the damaged @@ -189,40 +189,25 @@ where where E: TElement, { + use std::mem; + debug!("StylesheetSet::flush"); - let mut origins = OriginSet::empty(); - let mut have_invalidations = false; - - for (data, origin) in self.invalidation_data.iter_mut_origins() { - if data.dirty { - have_invalidations |= data.invalidations.flush(document_element); - data.dirty = false; - origins |= origin; - } - } + let have_invalidations = self.invalidations.flush(document_element); + let origins = mem::replace(&mut self.origins_dirty, OriginSet::empty()); (origins, have_invalidations) } /// Flush stylesheets, but without running any of the invalidation passes. - /// - /// FIXME(emilio): This should eventually disappear. Please keep this - /// Servo-only. #[cfg(feature = "servo")] - pub fn flush_without_invalidation(&mut self) -> (StylesheetIterator, OriginSet) { + pub fn flush_without_invalidation(&mut self) -> OriginSet { + use std::mem; + debug!("StylesheetSet::flush_without_invalidation"); - let mut origins = OriginSet::empty(); - for (data, origin) in self.invalidation_data.iter_mut_origins() { - if data.dirty { - data.invalidations.clear(); - data.dirty = false; - origins |= origin; - } - } - - (self.iter(), origins) + self.invalidations.clear(); + mem::replace(&mut self.origins_dirty, OriginSet::empty()) } /// Returns an iterator over the current list of stylesheets. @@ -233,30 +218,7 @@ where /// Mark the stylesheets for the specified origin as dirty, because /// something external may have invalidated it. pub fn force_dirty(&mut self, origins: OriginSet) { - for origin in origins.iter() { - let data = self.invalidation_data.borrow_mut_for_origin(&origin); - data.invalidations.invalidate_fully(); - data.dirty = true; - } - } -} - -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] -struct InvalidationData { - /// The stylesheet invalidations for this origin that we still haven't - /// processed. - invalidations: StylesheetInvalidationSet, - - /// Whether the sheets for this origin in the `StylesheetSet`'s entry list - /// has changed since the last restyle. - dirty: bool, -} - -impl Default for InvalidationData { - fn default() -> Self { - InvalidationData { - invalidations: StylesheetInvalidationSet::new(), - dirty: false, - } + self.invalidations.invalidate_fully(); + self.origins_dirty |= origins; } } diff --git a/components/style/stylesheets/origin.rs b/components/style/stylesheets/origin.rs index 1eb3c4932f1..29f5fdae16b 100644 --- a/components/style/stylesheets/origin.rs +++ b/components/style/stylesheets/origin.rs @@ -40,6 +40,7 @@ impl Origin { bitflags! { /// A set of origins. This is equivalent to Gecko's OriginFlags. + #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub flags OriginSet: u8 { /// https://drafts.csswg.org/css-cascade/#cascade-origin-user-agent const ORIGIN_USER_AGENT = Origin::UserAgent as u8, From 029ab24671bf4aad29d8759e65ac65b05323f4dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 22 Aug 2017 15:13:18 +0200 Subject: [PATCH 03/10] style: Add a no-op constructor of StyleSheetEntry, in preparation for more changes. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: 6Kz0S7YYmFC Signed-off-by: Emilio Cobos Álvarez --- components/style/stylesheet_set.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index 4158cf0b796..82872d4473f 100644 --- a/components/style/stylesheet_set.rs +++ b/components/style/stylesheet_set.rs @@ -14,13 +14,22 @@ use stylesheets::{Origin, OriginSet, StylesheetInDocument}; /// Entry for a StylesheetSet. We don't bother creating a constructor, because /// there's no sensible defaults for the member variables. #[cfg_attr(feature = "servo", derive(HeapSizeOf))] -pub struct StylesheetSetEntry +struct StylesheetSetEntry where S: StylesheetInDocument + PartialEq + 'static, { sheet: S, } +impl StylesheetSetEntry +where + S: StylesheetInDocument + PartialEq + 'static, +{ + fn new(sheet: S) -> Self { + Self { sheet } + } +} + /// A iterator over the stylesheets of a list of entries in the StylesheetSet. #[derive(Clone)] pub struct StylesheetIterator<'a, S>(slice::Iter<'a, StylesheetSetEntry>) @@ -119,7 +128,7 @@ where debug!("StylesheetSet::append_stylesheet"); self.remove_stylesheet_if_present(&sheet); self.collect_invalidations_for(device, &sheet, guard); - self.entries.push(StylesheetSetEntry { sheet }); + self.entries.push(StylesheetSetEntry::new(sheet)); } /// Prepend a new stylesheet to the current set. @@ -132,7 +141,7 @@ where debug!("StylesheetSet::prepend_stylesheet"); self.remove_stylesheet_if_present(&sheet); self.collect_invalidations_for(device, &sheet, guard); - self.entries.insert(0, StylesheetSetEntry { sheet }); + self.entries.insert(0, StylesheetSetEntry::new(sheet)); } /// Insert a given stylesheet before another stylesheet in the document. @@ -149,7 +158,7 @@ where entry.sheet == before_sheet }).expect("`before_sheet` stylesheet not found"); self.collect_invalidations_for(device, &sheet, guard); - self.entries.insert(index, StylesheetSetEntry { sheet }); + self.entries.insert(index, StylesheetSetEntry::new(sheet)); } /// Remove a given stylesheet from the set. From 15594dda10880ad3a35fc76d535f2da18255530a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 22 Aug 2017 15:48:38 +0200 Subject: [PATCH 04/10] style: Add a StylesheetFlusher abstraction to enable more incremental rebuilds. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: 8RBUUErm4bm Signed-off-by: Emilio Cobos Álvarez --- components/style/stylesheet_set.rs | 94 +++++++++++++++++++++++++++--- components/style/stylist.rs | 53 ++++++----------- 2 files changed, 105 insertions(+), 42 deletions(-) diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index 82872d4473f..386cc31c99f 100644 --- a/components/style/stylesheet_set.rs +++ b/components/style/stylesheet_set.rs @@ -47,6 +47,77 @@ where } } +/// A struct to iterate over the different stylesheets to be flushed. +pub struct StylesheetFlusher<'a, 'b, S> +where + 'b: 'a, + S: StylesheetInDocument + PartialEq + 'static, +{ + iter: slice::IterMut<'a, StylesheetSetEntry>, + guard: &'a SharedRwLockReadGuard<'b>, + origins_dirty: OriginSet, + author_style_disabled: bool, + had_invalidations: bool, +} + +/// The type of rebuild that we need to do for a given stylesheet. +pub enum SheetRebuildKind { + /// For now we only support full rebuilds, in the future we'll implement + /// partial rebuilds. + Full, +} + +impl<'a, 'b, S> StylesheetFlusher<'a, 'b, S> +where + 'b: 'a, + S: StylesheetInDocument + PartialEq + 'static, +{ + /// The set of origins to fully rebuild, which need to be cleared + /// beforehand. + pub fn origins_to_fully_rebuild(&self) -> OriginSet { + self.origins_dirty + } + + /// Returns whether running the whole flushing process would be a no-op. + pub fn nothing_to_do(&self) -> bool { + self.origins_dirty.is_empty() + } + + /// Returns whether any DOM invalidations were processed as a result of the + /// stylesheet flush. + pub fn had_invalidations(&self) -> bool { + self.had_invalidations + } +} + +impl<'a, 'b, S> Iterator for StylesheetFlusher<'a, 'b, S> +where + 'b: 'a, + S: StylesheetInDocument + PartialEq + 'static, +{ + type Item = (&'a S, SheetRebuildKind); + + fn next(&mut self) -> Option { + loop { + let potential_sheet = match self.iter.next() { + None => return None, + Some(s) => s, + }; + + let origin = potential_sheet.sheet.contents(self.guard).origin; + if !self.origins_dirty.contains(origin.into()) { + continue; + } + + if self.author_style_disabled && matches!(origin, Origin::Author) { + continue; + } + + return Some((&potential_sheet.sheet, SheetRebuildKind::Full)) + } + } +} + /// The set of stylesheets effective for a given document. #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct StylesheetSet @@ -189,12 +260,13 @@ where !self.origins_dirty.is_empty() } - /// Flush the current set, unmarking it as dirty, and returns the damaged - /// origins, and whether any elements were invalidated. - pub fn flush( - &mut self, + /// Flush the current set, unmarking it as dirty, and returns a + /// `StylesheetFlusher` in order to rebuild the stylist. + pub fn flush<'a, 'b, E>( + &'a mut self, document_element: Option, - ) -> (OriginSet, bool) + guard: &'a SharedRwLockReadGuard<'b>, + ) -> StylesheetFlusher<'a, 'b, S> where E: TElement, { @@ -202,10 +274,16 @@ where debug!("StylesheetSet::flush"); - let have_invalidations = self.invalidations.flush(document_element); - let origins = mem::replace(&mut self.origins_dirty, OriginSet::empty()); + let had_invalidations = self.invalidations.flush(document_element); + let origins_dirty = mem::replace(&mut self.origins_dirty, OriginSet::empty()); - (origins, have_invalidations) + StylesheetFlusher { + iter: self.entries.iter_mut(), + author_style_disabled: self.author_style_disabled, + had_invalidations, + origins_dirty, + guard, + } } /// Flush stylesheets, but without running any of the invalidation passes. diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 38b0861c047..5205958b9b8 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -38,7 +38,7 @@ use smallvec::VecLike; use std::fmt::Debug; use std::ops; use style_traits::viewport::ViewportConstraints; -use stylesheet_set::{StylesheetSet, StylesheetIterator}; +use stylesheet_set::{StylesheetSet, StylesheetIterator, StylesheetFlusher}; #[cfg(feature = "gecko")] use stylesheets::{CounterStyleRule, FontFaceRule}; use stylesheets::{CssRule, StyleRule}; @@ -83,22 +83,22 @@ impl DocumentCascadeData { /// Rebuild the cascade data for the given document stylesheets, and /// optionally with a set of user agent stylesheets. - fn rebuild<'a, I, S>( + fn rebuild<'a, 'b, S>( &mut self, device: &Device, quirks_mode: QuirksMode, - doc_stylesheets: I, + flusher: StylesheetFlusher<'a, 'b, S>, guards: &StylesheetGuards, ua_stylesheets: Option<&UserAgentStylesheets>, - author_style_disabled: bool, extra_data: &mut PerOrigin, - origins_to_rebuild: OriginSet, ) where - I: Iterator + Clone, - S: StylesheetInDocument + ToMediaListKey + 'static, + 'b: 'a, + S: StylesheetInDocument + ToMediaListKey + PartialEq + 'static, { - debug_assert!(!origins_to_rebuild.is_empty()); + debug_assert!(!flusher.nothing_to_do()); + + let origins_to_rebuild = flusher.origins_to_fully_rebuild(); for origin in origins_to_rebuild.iter() { extra_data.borrow_mut_for_origin(&origin).clear(); @@ -146,22 +146,13 @@ impl DocumentCascadeData { quirks_mode, &ua_stylesheets.quirks_mode_stylesheet, guards.ua_or_user, - extra_data + extra_data, ); } } } - // Only add stylesheets for origins we are updating, and only add - // Author level sheets if author style is not disabled. - let sheets_to_add = doc_stylesheets.filter(|s| { - let sheet_origin = s.contents(guards.author).origin; - - origins_to_rebuild.contains(sheet_origin.into()) && - (!matches!(sheet_origin, Origin::Author) || !author_style_disabled) - }); - - for stylesheet in sheets_to_add { + for (stylesheet, _rebuild_kind) in flusher { self.add_stylesheet( device, quirks_mode, @@ -489,16 +480,6 @@ impl Stylist { return false; } - let author_style_disabled = self.stylesheets.author_style_disabled(); - let (origins_to_rebuild, have_invalidations) = - self.stylesheets.flush(document_element); - - if origins_to_rebuild.is_empty() { - return have_invalidations; - } - - let doc_stylesheets = self.stylesheets.iter(); - self.num_rebuilds += 1; // Update viewport_constraints regardless of which origins' @@ -517,7 +498,9 @@ impl Stylist { // queries defined?) let cascaded_rule = ViewportRule { declarations: viewport_rule::Cascade::from_stylesheets( - doc_stylesheets.clone(), guards.author, &self.device + self.stylesheets.iter(), + guards.author, + &self.device, ).finish() }; @@ -533,18 +516,20 @@ impl Stylist { } } + let flusher = self.stylesheets.flush(document_element, &guards.author); + + let had_invalidations = flusher.had_invalidations(); + self.cascade_data.rebuild( &self.device, self.quirks_mode, - doc_stylesheets, + flusher, guards, ua_sheets, - author_style_disabled, extra_data, - origins_to_rebuild, ); - have_invalidations + had_invalidations } /// Insert a given stylesheet before another stylesheet in the document. From 1c67a7dc14b8b046d1b5c05e21cd7dc63a3aba32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 22 Aug 2017 15:57:13 +0200 Subject: [PATCH 05/10] style: Ensure the flusher is completely consumed before getting dropped. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will be necessary for future patches. MozReview-Commit-ID: FgWaoRCMdw2 Signed-off-by: Emilio Cobos Álvarez --- components/style/stylesheet_set.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index 386cc31c99f..104ada57783 100644 --- a/components/style/stylesheet_set.rs +++ b/components/style/stylesheet_set.rs @@ -90,6 +90,20 @@ where } } +#[cfg(debug_assertions)] +impl<'a, 'b, S> Drop for StylesheetFlusher<'a, 'b, S> +where + 'b: 'a, + S: StylesheetInDocument + PartialEq + 'static, +{ + fn drop(&mut self) { + debug_assert!( + self.iter.next().is_none(), + "You're supposed to fully consume the flusher", + ); + } +} + impl<'a, 'b, S> Iterator for StylesheetFlusher<'a, 'b, S> where 'b: 'a, From bf45a207602d368874035bc929e55f2df0c72036 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 22 Aug 2017 17:03:46 +0200 Subject: [PATCH 06/10] style: Implement finer-grained stylist rebuilds. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: ALsH9kSqHb0 Signed-off-by: Emilio Cobos Álvarez --- components/style/stylesheet_set.rs | 116 ++++++++++++++++++++++--- components/style/stylist.rs | 134 +++++++++++++++++++---------- 2 files changed, 193 insertions(+), 57 deletions(-) diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index 104ada57783..98abfbbb1c7 100644 --- a/components/style/stylesheet_set.rs +++ b/components/style/stylesheet_set.rs @@ -9,7 +9,7 @@ use invalidation::stylesheets::StylesheetInvalidationSet; use media_queries::Device; use shared_lock::SharedRwLockReadGuard; use std::slice; -use stylesheets::{Origin, OriginSet, StylesheetInDocument}; +use stylesheets::{Origin, OriginSet, PerOrigin, StylesheetInDocument}; /// Entry for a StylesheetSet. We don't bother creating a constructor, because /// there's no sensible defaults for the member variables. @@ -19,6 +19,7 @@ where S: StylesheetInDocument + PartialEq + 'static, { sheet: S, + dirty: bool, } impl StylesheetSetEntry @@ -26,7 +27,7 @@ where S: StylesheetInDocument + PartialEq + 'static, { fn new(sheet: S) -> Self { - Self { sheet } + Self { sheet, dirty: true } } } @@ -47,6 +48,28 @@ where } } +/// The validity of the data in a given cascade origin. +#[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd)] +#[cfg_attr(feature = "servo", derive(HeapSizeOf))] +pub enum OriginValidity { + /// The origin is clean, all the data already there is valid, though we may + /// have new sheets at the end. + Valid = 0, + + /// The cascade data is invalid, but not the invalidation data (which is + /// order-independent), and thus only the cascade data should be inserted. + CascadeInvalid = 1, + + /// Everything needs to be rebuilt. + FullyInvalid = 2, +} + +impl Default for OriginValidity { + fn default() -> Self { + OriginValidity::Valid + } +} + /// A struct to iterate over the different stylesheets to be flushed. pub struct StylesheetFlusher<'a, 'b, S> where @@ -56,15 +79,24 @@ where iter: slice::IterMut<'a, StylesheetSetEntry>, guard: &'a SharedRwLockReadGuard<'b>, origins_dirty: OriginSet, + origin_data_validity: PerOrigin, author_style_disabled: bool, had_invalidations: bool, } /// The type of rebuild that we need to do for a given stylesheet. pub enum SheetRebuildKind { - /// For now we only support full rebuilds, in the future we'll implement - /// partial rebuilds. + /// A full rebuild, of both cascade data and invalidation data. Full, + /// A partial rebuild, of only the cascade data. + CascadeOnly, +} + +impl SheetRebuildKind { + /// Whether the stylesheet invalidation data should be rebuilt. + pub fn rebuild_invalidation(&self) -> bool { + matches!(*self, SheetRebuildKind::Full) + } } impl<'a, 'b, S> StylesheetFlusher<'a, 'b, S> @@ -72,10 +104,9 @@ where 'b: 'a, S: StylesheetInDocument + PartialEq + 'static, { - /// The set of origins to fully rebuild, which need to be cleared - /// beforehand. - pub fn origins_to_fully_rebuild(&self) -> OriginSet { - self.origins_dirty + /// The data validity for a given origin. + pub fn origin_validity(&self, origin: Origin) -> OriginValidity { + *self.origin_data_validity.borrow_for_origin(&origin) } /// Returns whether running the whole flushing process would be a no-op. @@ -99,7 +130,7 @@ where fn drop(&mut self) { debug_assert!( self.iter.next().is_none(), - "You're supposed to fully consume the flusher", + "You're supposed to fully consume the flusher" ); } } @@ -112,12 +143,21 @@ where type Item = (&'a S, SheetRebuildKind); fn next(&mut self) -> Option { + use std::mem; + loop { let potential_sheet = match self.iter.next() { None => return None, Some(s) => s, }; + let dirty = mem::replace(&mut potential_sheet.dirty, false); + + if dirty { + // If the sheet was dirty, we need to do a full rebuild anyway. + return Some((&potential_sheet.sheet, SheetRebuildKind::Full)) + } + let origin = potential_sheet.sheet.contents(self.guard).origin; if !self.origins_dirty.contains(origin.into()) { continue; @@ -127,7 +167,13 @@ where continue; } - return Some((&potential_sheet.sheet, SheetRebuildKind::Full)) + let rebuild_kind = match self.origin_validity(origin) { + OriginValidity::Valid => continue, + OriginValidity::CascadeInvalid => SheetRebuildKind::CascadeOnly, + OriginValidity::FullyInvalid => SheetRebuildKind::Full, + }; + + return Some((&potential_sheet.sheet, rebuild_kind)); } } } @@ -151,6 +197,14 @@ where /// The origins whose stylesheets have changed so far. origins_dirty: OriginSet, + /// The validity of the data that was already there for a given origin. + /// + /// Note that an origin may appear on `origins_dirty`, but still have + /// `OriginValidity::Valid`, if only sheets have been appended into it (in + /// which case the existing data is valid, but the origin needs to be + /// rebuilt). + origin_data_validity: PerOrigin, + /// Has author style been disabled? author_style_disabled: bool, } @@ -165,6 +219,7 @@ where entries: vec![], invalidations: StylesheetInvalidationSet::new(), origins_dirty: OriginSet::empty(), + origin_data_validity: Default::default(), author_style_disabled: false, } } @@ -201,6 +256,24 @@ where self.origins_dirty |= sheet.contents(guard).origin; } + fn set_data_validity_at_least( + &mut self, + origin: Origin, + validity: OriginValidity, + ) { + use std::cmp; + + debug_assert!( + self.origins_dirty.contains(origin.into()), + "data_validity should be a subset of origins_dirty" + ); + + let existing_validity = + self.origin_data_validity.borrow_mut_for_origin(&origin); + + *existing_validity = cmp::max(*existing_validity, validity); + } + /// Appends a new stylesheet to the current set. /// /// No device implies not computing invalidations. @@ -213,6 +286,8 @@ where debug!("StylesheetSet::append_stylesheet"); self.remove_stylesheet_if_present(&sheet); self.collect_invalidations_for(device, &sheet, guard); + // Appending sheets doesn't alter the validity of the existing data, so + // we don't need to change `origin_data_validity` here. self.entries.push(StylesheetSetEntry::new(sheet)); } @@ -226,6 +301,11 @@ where debug!("StylesheetSet::prepend_stylesheet"); self.remove_stylesheet_if_present(&sheet); self.collect_invalidations_for(device, &sheet, guard); + + // Inserting stylesheets somewhere but at the end changes the validity + // of the cascade data, but not the invalidation data. + self.set_data_validity_at_least(sheet.contents(guard).origin, OriginValidity::CascadeInvalid); + self.entries.insert(0, StylesheetSetEntry::new(sheet)); } @@ -243,6 +323,10 @@ where entry.sheet == before_sheet }).expect("`before_sheet` stylesheet not found"); self.collect_invalidations_for(device, &sheet, guard); + + // Inserting stylesheets somewhere but at the end changes the validity + // of the cascade data, but not the invalidation data. + self.set_data_validity_at_least(sheet.contents(guard).origin, OriginValidity::CascadeInvalid); self.entries.insert(index, StylesheetSetEntry::new(sheet)); } @@ -255,7 +339,12 @@ where ) { debug!("StylesheetSet::remove_stylesheet"); self.remove_stylesheet_if_present(&sheet); + self.collect_invalidations_for(device, &sheet, guard); + + // Removing sheets makes us tear down the whole cascade and invalidation + // data. + self.set_data_validity_at_least(sheet.contents(guard).origin, OriginValidity::FullyInvalid); } /// Notes that the author style has been disabled for this document. @@ -290,12 +379,15 @@ where let had_invalidations = self.invalidations.flush(document_element); let origins_dirty = mem::replace(&mut self.origins_dirty, OriginSet::empty()); + let origin_data_validity = + mem::replace(&mut self.origin_data_validity, Default::default()); StylesheetFlusher { iter: self.entries.iter_mut(), author_style_disabled: self.author_style_disabled, had_invalidations, origins_dirty, + origin_data_validity, guard, } } @@ -321,5 +413,9 @@ where pub fn force_dirty(&mut self, origins: OriginSet) { self.invalidations.invalidate_fully(); self.origins_dirty |= origins; + for origin in origins.iter() { + // We don't know what happened, assume the worse. + self.set_data_validity_at_least(origin, OriginValidity::FullyInvalid); + } } } diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 5205958b9b8..6c719169b9c 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -38,7 +38,7 @@ use smallvec::VecLike; use std::fmt::Debug; use std::ops; use style_traits::viewport::ViewportConstraints; -use stylesheet_set::{StylesheetSet, StylesheetIterator, StylesheetFlusher}; +use stylesheet_set::{OriginValidity, SheetRebuildKind, StylesheetSet, StylesheetIterator, StylesheetFlusher}; #[cfg(feature = "gecko")] use stylesheets::{CounterStyleRule, FontFaceRule}; use stylesheets::{CssRule, StyleRule}; @@ -98,15 +98,24 @@ impl DocumentCascadeData { { debug_assert!(!flusher.nothing_to_do()); - let origins_to_rebuild = flusher.origins_to_fully_rebuild(); + for (cascade_data, origin) in self.per_origin.iter_mut_origins() { + let validity = flusher.origin_validity(origin); + + if validity == OriginValidity::Valid { + continue; + } + + if origin == Origin::UserAgent { + self.precomputed_pseudo_element_decls.clear(); + } - for origin in origins_to_rebuild.iter() { extra_data.borrow_mut_for_origin(&origin).clear(); - self.per_origin.borrow_mut_for_origin(&origin).clear(); - } - - if origins_to_rebuild.contains(Origin::UserAgent.into()) { - self.precomputed_pseudo_element_decls.clear(); + if validity == OriginValidity::CascadeInvalid { + cascade_data.clear_cascade_data() + } else { + debug_assert_eq!(validity, OriginValidity::FullyInvalid); + cascade_data.clear(); + } } if let Some(ua_stylesheets) = ua_stylesheets { @@ -119,15 +128,26 @@ impl DocumentCascadeData { Origin::UserAgent | Origin::User )); - if origins_to_rebuild.contains(sheet_origin.into()) { - self.add_stylesheet( - device, - quirks_mode, - stylesheet, - guards.ua_or_user, - extra_data, - ); + let validity = flusher.origin_validity(sheet_origin); + + // Servo doesn't support to incrementally mutate UA sheets. + debug_assert!(matches!( + validity, + OriginValidity::Valid | OriginValidity::FullyInvalid + )); + + if validity == OriginValidity::Valid { + continue; } + + self.add_stylesheet( + device, + quirks_mode, + stylesheet, + guards.ua_or_user, + extra_data, + SheetRebuildKind::Full, + ); } if quirks_mode != QuirksMode::NoQuirks { @@ -140,25 +160,35 @@ impl DocumentCascadeData { Origin::UserAgent | Origin::User )); - if origins_to_rebuild.contains(sheet_origin.into()) { + let validity = flusher.origin_validity(sheet_origin); + + // Servo doesn't support to incrementally mutate UA sheets. + debug_assert!(matches!( + validity, + OriginValidity::Valid | OriginValidity::FullyInvalid + )); + + if validity != OriginValidity::Valid { self.add_stylesheet( device, quirks_mode, &ua_stylesheets.quirks_mode_stylesheet, guards.ua_or_user, extra_data, + SheetRebuildKind::Full, ); } } } - for (stylesheet, _rebuild_kind) in flusher { + for (stylesheet, rebuild_kind) in flusher { self.add_stylesheet( device, quirks_mode, stylesheet, guards.author, - extra_data + extra_data, + rebuild_kind, ); } } @@ -169,7 +199,8 @@ impl DocumentCascadeData { quirks_mode: QuirksMode, stylesheet: &S, guard: &SharedRwLockReadGuard, - _extra_data: &mut PerOrigin + _extra_data: &mut PerOrigin, + rebuild_kind: SheetRebuildKind, ) where S: StylesheetInDocument + ToMediaListKey + 'static, @@ -183,9 +214,11 @@ impl DocumentCascadeData { let origin_cascade_data = self.per_origin.borrow_mut_for_origin(&origin); - origin_cascade_data - .effective_media_query_results - .saw_effective(stylesheet); + if rebuild_kind.rebuild_invalidation() { + origin_cascade_data + .effective_media_query_results + .saw_effective(stylesheet); + } for rule in stylesheet.effective_rules(device, guard) { match *rule { @@ -239,30 +272,32 @@ impl DocumentCascadeData { map.insert(rule, quirks_mode); - origin_cascade_data - .invalidation_map - .note_selector(selector, quirks_mode); - let mut visitor = StylistSelectorVisitor { - needs_revalidation: false, - passed_rightmost_selector: false, - attribute_dependencies: &mut origin_cascade_data.attribute_dependencies, - style_attribute_dependency: &mut origin_cascade_data.style_attribute_dependency, - state_dependencies: &mut origin_cascade_data.state_dependencies, - mapped_ids: &mut origin_cascade_data.mapped_ids, - }; + if rebuild_kind.rebuild_invalidation() { + origin_cascade_data + .invalidation_map + .note_selector(selector, quirks_mode); + let mut visitor = StylistSelectorVisitor { + needs_revalidation: false, + passed_rightmost_selector: false, + attribute_dependencies: &mut origin_cascade_data.attribute_dependencies, + style_attribute_dependency: &mut origin_cascade_data.style_attribute_dependency, + state_dependencies: &mut origin_cascade_data.state_dependencies, + mapped_ids: &mut origin_cascade_data.mapped_ids, + }; - selector.visit(&mut visitor); + selector.visit(&mut visitor); - if visitor.needs_revalidation { - origin_cascade_data.selectors_for_cache_revalidation.insert( - RevalidationSelectorAndHashes::new(selector.clone(), hashes), - quirks_mode - ); + if visitor.needs_revalidation { + origin_cascade_data.selectors_for_cache_revalidation.insert( + RevalidationSelectorAndHashes::new(selector.clone(), hashes), + quirks_mode + ); + } } } origin_cascade_data.rules_source_order += 1; } - CssRule::Import(ref lock) => { + CssRule::Import(ref lock) if rebuild_kind.rebuild_invalidation() => { let import_rule = lock.read_with(guard); origin_cascade_data .effective_media_query_results @@ -271,7 +306,7 @@ impl DocumentCascadeData { // NOTE: effective_rules visits the inner stylesheet if // appropriate. } - CssRule::Media(ref lock) => { + CssRule::Media(ref lock) if rebuild_kind.rebuild_invalidation() => { let media_rule = lock.read_with(guard); origin_cascade_data .effective_media_query_results @@ -1773,20 +1808,25 @@ impl CascadeData { self.pseudos_map.get(pseudo).is_some() } - fn clear(&mut self) { + /// Clears the cascade data, but not the invalidation data. + fn clear_cascade_data(&mut self) { self.element_map.clear(); self.pseudos_map.clear(); self.animations.clear(); + self.rules_source_order = 0; + self.num_selectors = 0; + self.num_declarations = 0; + } + + fn clear(&mut self) { + self.clear_cascade_data(); + self.effective_media_query_results.clear(); self.invalidation_map.clear(); self.attribute_dependencies.clear(); self.style_attribute_dependency = false; self.state_dependencies = ElementState::empty(); self.mapped_ids.clear(); self.selectors_for_cache_revalidation.clear(); - self.effective_media_query_results.clear(); - self.rules_source_order = 0; - self.num_selectors = 0; - self.num_declarations = 0; } } From 570da3b51291e3a402fd46bface81227bd9ae3d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 23 Aug 2017 14:02:35 +0200 Subject: [PATCH 07/10] style: Assert that only servo passes in UA sheets explicitly. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Emilio Cobos Álvarez --- components/style/stylist.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 6c719169b9c..f096f80733b 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -119,6 +119,8 @@ impl DocumentCascadeData { } if let Some(ua_stylesheets) = ua_stylesheets { + debug_assert!(cfg!(feature = "servo")); + for stylesheet in &ua_stylesheets.user_or_user_agent_stylesheets { let sheet_origin = stylesheet.contents(guards.ua_or_user).origin; From e648dd782269b7e5534cbc7bf5637a6712670ba9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 23 Aug 2017 14:06:14 +0200 Subject: [PATCH 08/10] style: Mention that it's worth to split CascadeData to make clearing clearer. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Emilio Cobos Álvarez --- components/style/stylist.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index f096f80733b..4496e6bb465 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1715,6 +1715,9 @@ impl<'a> SelectorVisitor for StylistSelectorVisitor<'a> { /// Data resulting from performing the CSS cascade that is specific to a given /// origin. +/// +/// FIXME(emilio): Consider renaming and splitting in `CascadeData` and +/// `InvalidationData`? That'd make `clear_cascade_data()` clearer. #[cfg_attr(feature = "servo", derive(HeapSizeOf))] #[derive(Debug)] struct CascadeData { From 3fd8a534d7f657b4b9df026297834dee0a091534 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 23 Aug 2017 14:06:44 +0200 Subject: [PATCH 09/10] style: Rename SheetRebuildKind::rebuild_invalidation to should_rebuild_invalidation. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Emilio Cobos Álvarez --- components/style/stylesheet_set.rs | 2 +- components/style/stylist.rs | 28 ++++++++++++++++------------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index 98abfbbb1c7..eb444b556f5 100644 --- a/components/style/stylesheet_set.rs +++ b/components/style/stylesheet_set.rs @@ -94,7 +94,7 @@ pub enum SheetRebuildKind { impl SheetRebuildKind { /// Whether the stylesheet invalidation data should be rebuilt. - pub fn rebuild_invalidation(&self) -> bool { + pub fn should_rebuild_invalidation(&self) -> bool { matches!(*self, SheetRebuildKind::Full) } } diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 4496e6bb465..bd3c30cce05 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -216,7 +216,7 @@ impl DocumentCascadeData { let origin_cascade_data = self.per_origin.borrow_mut_for_origin(&origin); - if rebuild_kind.rebuild_invalidation() { + if rebuild_kind.should_rebuild_invalidation() { origin_cascade_data .effective_media_query_results .saw_effective(stylesheet); @@ -274,7 +274,7 @@ impl DocumentCascadeData { map.insert(rule, quirks_mode); - if rebuild_kind.rebuild_invalidation() { + if rebuild_kind.should_rebuild_invalidation() { origin_cascade_data .invalidation_map .note_selector(selector, quirks_mode); @@ -299,20 +299,24 @@ impl DocumentCascadeData { } origin_cascade_data.rules_source_order += 1; } - CssRule::Import(ref lock) if rebuild_kind.rebuild_invalidation() => { - let import_rule = lock.read_with(guard); - origin_cascade_data - .effective_media_query_results - .saw_effective(import_rule); + CssRule::Import(ref lock) => { + if rebuild_kind.should_rebuild_invalidation() { + let import_rule = lock.read_with(guard); + origin_cascade_data + .effective_media_query_results + .saw_effective(import_rule); + } // NOTE: effective_rules visits the inner stylesheet if // appropriate. } - CssRule::Media(ref lock) if rebuild_kind.rebuild_invalidation() => { - let media_rule = lock.read_with(guard); - origin_cascade_data - .effective_media_query_results - .saw_effective(media_rule); + CssRule::Media(ref lock) => { + if rebuild_kind.should_rebuild_invalidation() { + let media_rule = lock.read_with(guard); + origin_cascade_data + .effective_media_query_results + .saw_effective(media_rule); + } } CssRule::Keyframes(ref keyframes_rule) => { let keyframes_rule = keyframes_rule.read_with(guard); From fca2f48f8666280a163b76d6ee02b7ce4c89ce22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 23 Aug 2017 14:07:34 +0200 Subject: [PATCH 10/10] style: Remove outdated comment. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Emilio Cobos Álvarez --- components/style/stylesheet_set.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index eb444b556f5..01e8bedc133 100644 --- a/components/style/stylesheet_set.rs +++ b/components/style/stylesheet_set.rs @@ -11,8 +11,7 @@ use shared_lock::SharedRwLockReadGuard; use std::slice; use stylesheets::{Origin, OriginSet, PerOrigin, StylesheetInDocument}; -/// Entry for a StylesheetSet. We don't bother creating a constructor, because -/// there's no sensible defaults for the member variables. +/// Entry for a StylesheetSet. #[cfg_attr(feature = "servo", derive(HeapSizeOf))] struct StylesheetSetEntry where