Bug 1458192: Undo the packing in ApplicableDeclarationBlock. draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 01 May 2018 09:21:07 +0200
changeset 790085 91555aac14937f8bc6f29529b8867490c1c1f723
parent 790084 b4fadc7ec94af24a20bb8b981a485a3c1f112687
child 790086 bc8b332d766102331eb6a795e0c8201d115a5379
push id108423
push userbmo:emilio@crisal.io
push dateTue, 01 May 2018 10:06:21 +0000
bugs1458192
milestone61.0a1
Bug 1458192: Undo the packing in ApplicableDeclarationBlock. It will get in the way of the next refactor, which puts the shadow order in the cascade level. It's not a regression with respect of what we had last week anyway, and we can always pack them again in a smart way, but I doubt it's that worth it, and it was more unsafe code which I'd rather avoid. MozReview-Commit-ID: 81mWF3GP86h
servo/components/style/applicable_declarations.rs
--- a/servo/components/style/applicable_declarations.rs
+++ b/servo/components/style/applicable_declarations.rs
@@ -4,158 +4,94 @@
 
 //! Applicable declarations management.
 
 use properties::PropertyDeclarationBlock;
 use rule_tree::{CascadeLevel, ShadowCascadeOrder, StyleSource};
 use servo_arc::Arc;
 use shared_lock::Locked;
 use smallvec::SmallVec;
-use std::fmt::{self, Debug};
 
 /// List of applicable declarations. This is a transient structure that shuttles
 /// declarations between selector matching and inserting into the rule tree, and
 /// therefore we want to avoid heap-allocation where possible.
 ///
 /// In measurements on wikipedia, we pretty much never have more than 8 applicable
 /// declarations, so we could consider making this 8 entries instead of 16.
 /// However, it may depend a lot on workload, and stack space is cheap.
 pub type ApplicableDeclarationList = SmallVec<[ApplicableDeclarationBlock; 16]>;
 
-/// Blink uses 18 bits to store source order, and does not check overflow [1].
-/// That's a limit that could be reached in realistic webpages, so we use
-/// 24 bits and enforce defined behavior in the overflow case.
-///
-/// [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/
-///     RuleSet.h?l=128&rcl=90140ab80b84d0f889abc253410f44ed54ae04f3
-const SOURCE_ORDER_SHIFT: usize = 0;
-const SOURCE_ORDER_BITS: usize = 24;
-const SOURCE_ORDER_MAX: u32 = (1 << SOURCE_ORDER_BITS) - 1;
-const SOURCE_ORDER_MASK: u32 = SOURCE_ORDER_MAX << SOURCE_ORDER_SHIFT;
-
-/// We store up-to-15 shadow order levels.
-///
-/// You'd need an element slotted across 16 components with ::slotted rules to
-/// trigger this as of this writing, which looks... Unlikely.
-const SHADOW_CASCADE_ORDER_SHIFT: usize = SOURCE_ORDER_BITS;
-const SHADOW_CASCADE_ORDER_BITS: usize = 4;
-const SHADOW_CASCADE_ORDER_MAX: u8 = (1 << SHADOW_CASCADE_ORDER_BITS) - 1;
-const SHADOW_CASCADE_ORDER_MASK: u32 = (SHADOW_CASCADE_ORDER_MAX as u32) << SHADOW_CASCADE_ORDER_SHIFT;
-
-const CASCADE_LEVEL_SHIFT: usize = SOURCE_ORDER_BITS + SHADOW_CASCADE_ORDER_BITS;
-const CASCADE_LEVEL_BITS: usize = 4;
-const CASCADE_LEVEL_MAX: u8 = (1 << CASCADE_LEVEL_BITS) - 1;
-const CASCADE_LEVEL_MASK: u32 = (CASCADE_LEVEL_MAX as u32) << CASCADE_LEVEL_SHIFT;
-
-/// Stores the source order of a block, the cascade level it belongs to, and the
-/// counter needed to handle Shadow DOM cascade order properly.
-#[derive(Clone, Copy, Eq, MallocSizeOf, PartialEq)]
-struct ApplicableDeclarationBits(u32);
-
-impl ApplicableDeclarationBits {
-    fn new(
-        source_order: u32,
-        cascade_level: CascadeLevel,
-        shadow_cascade_order: ShadowCascadeOrder,
-    ) -> Self {
-        debug_assert!(
-            cascade_level as u8 <= CASCADE_LEVEL_MAX,
-            "Gotta find more bits!"
-        );
-        let mut bits = ::std::cmp::min(source_order, SOURCE_ORDER_MAX);
-        bits |= ((shadow_cascade_order.unwrap() & SHADOW_CASCADE_ORDER_MAX) as u32) << SHADOW_CASCADE_ORDER_SHIFT;
-        bits |= (cascade_level as u8 as u32) << CASCADE_LEVEL_SHIFT;
-        ApplicableDeclarationBits(bits)
-    }
-
-    fn source_order(&self) -> u32 {
-        (self.0 & SOURCE_ORDER_MASK) >> SOURCE_ORDER_SHIFT
-    }
-
-    fn shadow_cascade_order(&self) -> ShadowCascadeOrder {
-        ShadowCascadeOrder::wrap(((self.0 & SHADOW_CASCADE_ORDER_MASK) >> SHADOW_CASCADE_ORDER_SHIFT) as u8)
-    }
-
-    fn level(&self) -> CascadeLevel {
-        let byte = ((self.0 & CASCADE_LEVEL_MASK) >> CASCADE_LEVEL_SHIFT) as u8;
-        unsafe { CascadeLevel::from_byte(byte) }
-    }
-}
-
-impl Debug for ApplicableDeclarationBits {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        f.debug_struct("ApplicableDeclarationBits")
-            .field("source_order", &self.source_order())
-            .field("shadow_cascade_order", &self.shadow_cascade_order())
-            .field("level", &self.level())
-            .finish()
-    }
-}
-
 /// A property declaration together with its precedence among rules of equal
 /// specificity so that we can sort them.
 ///
 /// This represents the declarations in a given declaration block for a given
 /// importance.
 #[derive(Clone, Debug, MallocSizeOf, PartialEq)]
 pub struct ApplicableDeclarationBlock {
     /// The style source, either a style rule, or a property declaration block.
     #[ignore_malloc_size_of = "Arc"]
     pub source: StyleSource,
-    /// The bits containing the source order, cascade level, and shadow cascade
-    /// order.
-    bits: ApplicableDeclarationBits,
+    /// The source order of the selector.
+    source_order: u32,
     /// The specificity of the selector this block is represented by.
     pub specificity: u32,
+    /// The cascade level of the rule.
+    cascade_level: CascadeLevel,
+    /// The shadow cascade order of the rule.
+    shadow_cascade_order: ShadowCascadeOrder,
 }
 
 impl ApplicableDeclarationBlock {
     /// Constructs an applicable declaration block from a given property
     /// declaration block and importance.
     #[inline]
     pub fn from_declarations(
         declarations: Arc<Locked<PropertyDeclarationBlock>>,
-        level: CascadeLevel,
+        cascade_level: CascadeLevel,
     ) -> Self {
         ApplicableDeclarationBlock {
             source: StyleSource::from_declarations(declarations),
-            bits: ApplicableDeclarationBits::new(0, level, ShadowCascadeOrder::same_tree()),
+            source_order: 0,
+            cascade_level,
+            shadow_cascade_order: ShadowCascadeOrder::same_tree(),
             specificity: 0,
         }
     }
 
     /// Constructs an applicable declaration block from the given components
     #[inline]
     pub fn new(
         source: StyleSource,
-        order: u32,
-        level: CascadeLevel,
+        source_order: u32,
+        cascade_level: CascadeLevel,
+        shadow_cascade_order: ShadowCascadeOrder,
         specificity: u32,
-        shadow_cascade_order: ShadowCascadeOrder,
     ) -> Self {
         ApplicableDeclarationBlock {
             source,
-            bits: ApplicableDeclarationBits::new(order, level, shadow_cascade_order),
+            source_order,
+            cascade_level,
+            shadow_cascade_order,
             specificity,
         }
     }
 
     /// Returns the source order of the block.
     #[inline]
     pub fn source_order(&self) -> u32 {
-        self.bits.source_order()
+        self.source_order
     }
 
     /// Returns the cascade level of the block.
     #[inline]
     pub fn level(&self) -> CascadeLevel {
-        self.bits.level()
+        self.cascade_level
     }
 
     /// Convenience method to consume self and return the right thing for the
     /// rule tree to iterate over.
     #[inline]
     pub fn for_rule_tree(self) -> (StyleSource, CascadeLevel, ShadowCascadeOrder) {
         let level = self.level();
-        let cascade_order = self.bits.shadow_cascade_order();
+        let cascade_order = self.shadow_cascade_order;
         (self.source, level, cascade_order)
     }
 }