Bug 1386602: Avoid recreating the stylist in RebuildAllStyleData. draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 02 Aug 2017 14:39:32 +0200
changeset 620582 225b7d1cc9d11b9ef574643d192a5e8658dd3e7d
parent 620581 8b1718ccd719177409b7fa42f7dc96f7d2c1438c
child 640745 138c1ab8cc309ff3d7bacc21089fbde65d2b5fe9
push id72089
push userbmo:emilio+bugs@crisal.io
push dateThu, 03 Aug 2017 16:11:32 +0000
bugs1386602
milestone57.0a1
Bug 1386602: Avoid recreating the stylist in RebuildAllStyleData. MozReview-Commit-ID: 31G9BLgqEmm
layout/base/ServoRestyleManager.cpp
layout/style/ServoBindingList.h
layout/style/ServoStyleSet.cpp
layout/style/ServoStyleSet.h
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -214,17 +214,21 @@ ServoRestyleManager::RebuildAllStyleData
    // to be needed in my testing.
   PostRebuildAllStyleDataEvent(aExtraHint, aRestyleHint);
 }
 
 void
 ServoRestyleManager::PostRebuildAllStyleDataEvent(nsChangeHint aExtraHint,
                                                   nsRestyleHint aRestyleHint)
 {
-  StyleSet()->ClearDataAndMarkDeviceDirty();
+  // NOTE(emilio): The semantics of these methods are quite funny, in the sense
+  // that we're not supposed to need to rebuild the actual stylist data.
+  //
+  // That's handled as part of the MediumFeaturesChanged stuff, if needed.
+  StyleSet()->ClearCachedStyleData();
 
   DocumentStyleRootIterator iter(mPresContext->Document());
   while (Element* root = iter.GetNextStyleRoot()) {
     PostRestyleEvent(root, aRestyleHint, aExtraHint);
   }
 
   // TODO(emilio, bz): Extensions can add/remove stylesheets that can affect
   // non-inheriting anon boxes. It's not clear if we want to support that, but
--- a/layout/style/ServoBindingList.h
+++ b/layout/style/ServoBindingList.h
@@ -42,20 +42,20 @@ SERVO_BINDING_FUNC(Servo_StyleSheet_Clon
                    RawServoStyleSheetContentsBorrowed sheet,
                    const mozilla::ServoStyleSheet* reference_sheet);
 SERVO_BINDING_FUNC(Servo_StyleSheet_SizeOfIncludingThis, size_t,
                    mozilla::MallocSizeOf malloc_size_of,
                    RawServoStyleSheetContentsBorrowed sheet)
 SERVO_BINDING_FUNC(Servo_StyleSet_Init, RawServoStyleSetOwned, RawGeckoPresContextOwned pres_context)
 SERVO_BINDING_FUNC(Servo_StyleSet_Clear, void,
                    RawServoStyleSetBorrowed set)
-SERVO_BINDING_FUNC(Servo_StyleSet_RebuildData, void,
+SERVO_BINDING_FUNC(Servo_StyleSet_RebuildCachedData, void,
                    RawServoStyleSetBorrowed set)
-SERVO_BINDING_FUNC(Servo_StyleSet_MediumFeaturesChanged, nsRestyleHint,
-                   RawServoStyleSetBorrowed set, bool viewport_changed)
+SERVO_BINDING_FUNC(Servo_StyleSet_MediumFeaturesChanged, bool,
+                   RawServoStyleSetBorrowed set, bool* viewport_units_used)
 SERVO_BINDING_FUNC(Servo_StyleSet_Drop, void, RawServoStyleSetOwned set)
 SERVO_BINDING_FUNC(Servo_StyleSet_CompatModeChanged, void,
                    RawServoStyleSetBorrowed raw_data)
 SERVO_BINDING_FUNC(Servo_StyleSet_AppendStyleSheet, void,
                    RawServoStyleSetBorrowed set,
                    const mozilla::ServoStyleSheet* gecko_sheet)
 SERVO_BINDING_FUNC(Servo_StyleSet_PrependStyleSheet, void,
                    RawServoStyleSetBorrowed set,
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -109,19 +109,32 @@ ServoStyleSet::Shutdown()
 void
 ServoStyleSet::InvalidateStyleForCSSRuleChanges()
 {
   MOZ_ASSERT(StylistNeedsUpdate());
   mPresContext->RestyleManager()->AsServo()->PostRestyleEventForCSSRuleChanges();
 }
 
 nsRestyleHint
-ServoStyleSet::MediumFeaturesChanged(bool aViewportChanged) const
+ServoStyleSet::MediumFeaturesChanged(bool aViewportChanged)
 {
-  return Servo_StyleSet_MediumFeaturesChanged(mRawSet.get(), aViewportChanged);
+  bool viewportUnitsUsed = false;
+  const bool rulesChanged =
+    Servo_StyleSet_MediumFeaturesChanged(mRawSet.get(), &viewportUnitsUsed);
+
+  if (rulesChanged) {
+    ForceAllStyleDirty();
+    return eRestyle_Subtree;
+  }
+
+  if (viewportUnitsUsed && aViewportChanged) {
+    return eRestyle_ForceDescendants;
+  }
+
+  return nsRestyleHint(0);
 }
 
 size_t
 ServoStyleSet::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
 {
   size_t n = aMallocSizeOf(this);
 
   if (mStyleRuleMap) {
@@ -989,28 +1002,20 @@ ServoStyleSet::EnsureUniqueInnerOnCSSShe
   }
 
   bool res = mNeedsRestyleAfterEnsureUniqueInner;
   mNeedsRestyleAfterEnsureUniqueInner = false;
   return res;
 }
 
 void
-ServoStyleSet::RebuildData()
+ServoStyleSet::ClearCachedStyleData()
 {
   ClearNonInheritingStyleContexts();
-  Servo_StyleSet_RebuildData(mRawSet.get());
-}
-
-void
-ServoStyleSet::ClearDataAndMarkDeviceDirty()
-{
-  ClearNonInheritingStyleContexts();
-  Servo_StyleSet_Clear(mRawSet.get());
-  mStylistState |= StylistState::FullyDirty;
+  Servo_StyleSet_RebuildCachedData(mRawSet.get());
 }
 
 void
 ServoStyleSet::CompatibilityModeChanged()
 {
   Servo_StyleSet_CompatModeChanged(mRawSet.get());
 }
 
@@ -1146,41 +1151,18 @@ ServoStyleSet::ResolveForDeclarations(
                                                aParentOrNull,
                                                aDeclarations).Consume();
 }
 
 void
 ServoStyleSet::UpdateStylist()
 {
   MOZ_ASSERT(StylistNeedsUpdate());
-  if (mStylistState & StylistState::FullyDirty) {
-    RebuildData();
-
-    if (mStylistState & StylistState::StyleSheetsDirty) {
-      // Normally, whoever was in charge of posting a RebuildAllStyleDataEvent,
-      // would also be in charge of posting a restyle/change hint according to
-      // it.
-      //
-      // However, other stylesheets may have been added to the document in the
-      // same period, so when both bits are set, we need to do a full subtree
-      // update, because we can no longer reason about the state of the style
-      // data.
-      //
-      // We could not clear the invalidations when rebuilding the data and
-      // process them here... But it's not clear if that complexity is worth
-      // to handle this edge case more efficiently.
-      if (Element* root = mPresContext->Document()->GetDocumentElement()) {
-        Servo_NoteExplicitHints(root, eRestyle_Subtree, nsChangeHint(0));
-      }
-    }
-  } else {
-    MOZ_ASSERT(mStylistState & StylistState::StyleSheetsDirty);
-    Element* root = mPresContext->Document()->GetDocumentElement();
-    Servo_StyleSet_FlushStyleSheets(mRawSet.get(), root);
-  }
+  Element* root = mPresContext->Document()->GetDocumentElement();
+  Servo_StyleSet_FlushStyleSheets(mRawSet.get(), root);
   mStylistState = StylistState::NotDirty;
 }
 
 void
 ServoStyleSet::MaybeGCRuleTree()
 {
   MOZ_ASSERT(NS_IsMainThread());
   Servo_MaybeGCRuleTree(mRawSet.get());
--- a/layout/style/ServoStyleSet.h
+++ b/layout/style/ServoStyleSet.h
@@ -50,27 +50,19 @@ namespace mozilla {
  * A few flags used to track which kind of stylist state we may need to
  * update.
  */
 enum class StylistState : uint8_t {
   /** The stylist is not dirty, we should do nothing */
   NotDirty = 0,
 
   /** The style sheets have changed, so we need to update the style data. */
-  StyleSheetsDirty = 1 << 0,
-
-  /**
-   * All style data is dirty and both style sheet data and default computed
-   * values need to be recomputed.
-   */
-  FullyDirty = 1 << 1,
+  StyleSheetsDirty,
 };
 
-MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(StylistState)
-
 /**
  * The set of style sheets that apply to a document, backed by a Servo
  * Stylist.  A ServoStyleSet contains ServoStyleSheets.
  */
 class ServoStyleSet
 {
   friend class ServoRestyleManager;
   typedef ServoElementSnapshotTable SnapshotTable;
@@ -110,17 +102,17 @@ public:
     ForceAllStyleDirty();
   }
 
   bool StyleSheetsHaveChanged() const
   {
     return StylistNeedsUpdate();
   }
 
-  nsRestyleHint MediumFeaturesChanged(bool aViewportChanged) const;
+  nsRestyleHint MediumFeaturesChanged(bool aViewportChanged);
 
   void InvalidateStyleForCSSRuleChanges();
 
   size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
   const RawServoStyleSet* RawSet() const {
     return mRawSet.get();
   }
 
@@ -308,17 +300,17 @@ public:
   /**
    * Records that the contents of style sheets have changed since the last
    * restyle.  Calling this will ensure that the Stylist rebuilds its
    * selector maps.
    */
   void ForceAllStyleDirty();
 
   /**
-   * Helper for correctly calling RebuildStylist without paying the cost of an
+   * Helper for correctly calling UpdateStylist without paying the cost of an
    * extra function call in the common no-rebuild-needed case.
    */
   void UpdateStylistIfNeeded()
   {
     if (StylistNeedsUpdate()) {
       UpdateStylist();
     }
   }
@@ -340,21 +332,25 @@ public:
 
 #ifdef DEBUG
   void AssertTreeIsClean();
 #else
   void AssertTreeIsClean() {}
 #endif
 
   /**
-   * Clears the style data, both style sheet data and cached non-inheriting
-   * style contexts, and marks the stylist as needing an unconditional full
-   * rebuild, including a device reset.
+   * Clears any cached style data that may depend on all sorts of computed
+   * values.
+   *
+   * Right now this clears the non-inheriting style context cache, and resets
+   * the default computed values.
+   *
+   * This does _not_, however, clear the stylist.
    */
-  void ClearDataAndMarkDeviceDirty();
+  void ClearCachedStyleData();
 
   /**
    * Notifies the Servo stylesheet that the document's compatibility mode has changed.
    */
   void CompatibilityModeChanged();
 
   /**
    * Resolve style for the given element, and return it as a
@@ -490,22 +486,16 @@ private:
       mSet->RunPostTraversalTasks();
     }
 
   private:
     ServoStyleSet* mSet;
   };
 
   /**
-   * Rebuild the style data. This will force a stylesheet flush, and also
-   * recompute the default computed styles.
-   */
-  void RebuildData();
-
-  /**
    * Gets the pending snapshots to handle from the restyle manager.
    */
   const SnapshotTable& Snapshots();
 
   /**
    * Resolve all ServoDeclarationBlocks attached to mapped
    * presentation attributes cached on the document.
    *
@@ -543,17 +533,17 @@ private:
   // Subset of the pre-traverse steps that involve syncing up data
   void PreTraverseSync();
 
   /**
    * Note that the stylist needs a style flush due to style sheet changes.
    */
   void SetStylistStyleSheetsDirty()
   {
-    mStylistState |= StylistState::StyleSheetsDirty;
+    mStylistState = StylistState::StyleSheetsDirty;
   }
 
   bool StylistNeedsUpdate() const
   {
     return mStylistState != StylistState::NotDirty;
   }
 
   /**