Bug 1302054 - Part 1: Avoid computing style differences when we just want to ensure structs are cached on the new context. r=dbaron draft
authorCameron McCormack <cam@mcc.id.au>
Thu, 16 Mar 2017 17:04:32 +0800
changeset 499848 d9fca81bf76cd90fddc3096d9bd7613749141125
parent 499847 fea4c36eaf6e8f85aabbc31182830d76018542d8
child 499849 8a385b0ae83eafffcf438514f8213c53309d822a
push id49561
push userbmo:cam@mcc.id.au
push dateThu, 16 Mar 2017 09:05:22 +0000
reviewersdbaron
bugs1302054
milestone55.0a1
Bug 1302054 - Part 1: Avoid computing style differences when we just want to ensure structs are cached on the new context. r=dbaron MozReview-Commit-ID: DLhHcCD4GQS * * * [mq]: x MozReview-Commit-ID: 7XOgQYu9kiW
layout/base/GeckoRestyleManager.cpp
layout/base/ServoRestyleManager.cpp
layout/style/nsStyleContext.cpp
layout/style/nsStyleContext.h
--- a/layout/base/GeckoRestyleManager.cpp
+++ b/layout/base/GeckoRestyleManager.cpp
@@ -944,31 +944,20 @@ GeckoRestyleManager::ReparentStyleContex
       // nsTransitionManager::ConsiderInitiatingTransition.
 #if 0
       if (!copyFromContinuation) {
         TryInitiatingTransition(mPresContext, aFrame->GetContent(),
                                 oldContext, &newContext);
       }
 #endif
 
-      // Make sure to call CalcStyleDifference so that the new context ends
-      // up resolving all the structs the old context resolved.
+      // Ensure the new context ends up resolving all the structs the old
+      // context resolved.
       if (!copyFromContinuation) {
-        uint32_t equalStructs;
-        uint32_t samePointerStructs;
-        DebugOnly<nsChangeHint> styleChange =
-          oldContext->CalcStyleDifference(newContext, nsChangeHint(0),
-                                          &equalStructs,
-                                          &samePointerStructs);
-        // The style change is always 0 because we have the same rulenode and
-        // CalcStyleDifference optimizes us away.  That's OK, though:
-        // reparenting should never trigger a frame reconstruct, and whenever
-        // it's happening we already plan to reflow and repaint the frames.
-        NS_ASSERTION(!(styleChange & nsChangeHint_ReconstructFrame),
-                     "Our frame tree is likely to be bogus!");
+        newContext->EnsureSameStructsCached(oldContext);
       }
 
       aFrame->SetStyleContext(newContext);
 
       nsIFrame::ChildListIterator lists(aFrame);
       for (; !lists.IsDone(); lists.Next()) {
         for (nsIFrame* child : lists.CurrentList()) {
           // only do frames that are in flow
@@ -1010,33 +999,19 @@ GeckoRestyleManager::ReparentStyleContex
            (oldExtraContext = aFrame->GetAdditionalStyleContext(contextIndex));
            ++contextIndex) {
         RefPtr<nsStyleContext> newExtraContext;
         newExtraContext = StyleSet()->
                             ReparentStyleContext(oldExtraContext,
                                                  newContext, nullptr);
         if (newExtraContext) {
           if (newExtraContext != oldExtraContext) {
-            // Make sure to call CalcStyleDifference so that the new
-            // context ends up resolving all the structs the old context
-            // resolved.
-            uint32_t equalStructs;
-            uint32_t samePointerStructs;
-            DebugOnly<nsChangeHint> styleChange =
-              oldExtraContext->CalcStyleDifference(newExtraContext,
-                                                   nsChangeHint(0),
-                                                   &equalStructs,
-                                                   &samePointerStructs);
-            // The style change is always 0 because we have the same
-            // rulenode and CalcStyleDifference optimizes us away.  That's
-            // OK, though: reparenting should never trigger a frame
-            // reconstruct, and whenever it's happening we already plan to
-            // reflow and repaint the frames.
-            NS_ASSERTION(!(styleChange & nsChangeHint_ReconstructFrame),
-                         "Our frame tree is likely to be bogus!");
+            // Ensure the new context ends up resolving all the structs the old
+            // context resolved.
+            newContext->EnsureSameStructsCached(oldContext);
           }
 
           aFrame->SetAdditionalStyleContext(contextIndex, newExtraContext);
         }
       }
 #ifdef DEBUG
       DebugVerifyStyleTree(aFrame);
 #endif
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -215,17 +215,17 @@ ServoRestyleManager::ProcessPostTraversa
 
   RefPtr<nsStyleContext> newContext = nullptr;
   if (recreateContext) {
     MOZ_ASSERT(styleFrame || displayContentsNode);
     newContext =
       aStyleSet->GetContext(computedValues.forget(), aParentContext, nullptr,
                             CSSPseudoElementType::NotPseudo, aElement);
 
-    newContext->EnsureStructsForServo(oldStyleContext);
+    newContext->EnsureSameStructsCached(oldStyleContext);
 
     // XXX This could not always work as expected: there are kinds of content
     // with the first split and the last sharing style, but others not. We
     // should handle those properly.
     // XXXbz I think the UpdateStyleOfOwnedAnonBoxes call below handles _that_
     // right, but not other cases where we happen to have different styles on
     // different continuations... (e.g. first-line).
     for (nsIFrame* f = styleFrame; f;
--- a/layout/style/nsStyleContext.cpp
+++ b/layout/style/nsStyleContext.cpp
@@ -174,46 +174,16 @@ nsStyleContext::FinishConstruction(bool 
   }
 
   #define eStyleStruct_LastItem (nsStyleStructID_Length - 1)
   NS_ASSERTION(NS_STYLE_INHERIT_MASK & NS_STYLE_INHERIT_BIT(LastItem),
                "NS_STYLE_INHERIT_MASK must be bigger, and other bits shifted");
   #undef eStyleStruct_LastItem
 }
 
-void
-nsStyleContext::EnsureStructsForServo(const nsStyleContext* aOldContext)
-{
-  MOZ_ASSERT(aOldContext);
-  MOZ_ASSERT(mSource.IsServoComputedValues() &&
-             aOldContext->mSource.IsServoComputedValues());
-
-  // NOTE(emilio): We could do better here. We only call Style##name_() because
-  // we need to run FinishStyle, but otherwise this is only a bitwise or.
-  //
-  // We could reduce the FFI traffic we do only doing it for structs that have
-  // non-trivial FinishStyle.
-#define STYLE_STRUCT(name_, checkdata_cb)                                      \
-  if (aOldContext->mBits & NS_STYLE_INHERIT_BIT(name_)) {                      \
-    mozilla::Unused << Style##name_();                                         \
-  }
-
-#include "nsStyleStructList.h"
-
-#undef STYLE_STRUCT
-
-#ifdef DEBUG
-  auto oldMask = aOldContext->mBits & NS_STYLE_INHERIT_MASK;
-  auto newMask = mBits & NS_STYLE_INHERIT_MASK;
-  MOZ_ASSERT((oldMask & newMask) == oldMask,
-             "Should have at least as many structs computed as the "
-             "old context!");
-#endif
-}
-
 nsStyleContext::~nsStyleContext()
 {
   NS_ASSERTION((nullptr == mChild) && (nullptr == mEmptyChild), "destructing context with children");
   MOZ_ASSERT_IF(mSource.IsServoComputedValues(), !mCachedResetData);
 
 #ifdef DEBUG
   if (mSource.IsServoComputedValues()) {
     MOZ_ASSERT(!mCachedResetData,
@@ -1326,16 +1296,42 @@ nsStyleContext::CalcStyleDifference(cons
 {
   FakeStyleContext newContext(aNewComputedValues);
   return CalcStyleDifferenceInternal(&newContext,
                                      aParentHintsNotHandledForDescendants,
                                      aEqualStructs,
                                      aSamePointerStructs);
 }
 
+void
+nsStyleContext::EnsureSameStructsCached(nsStyleContext* aOldContext)
+{
+  // NOTE(emilio): We could do better here for stylo, where we only call
+  // Style##name_() because we need to run FinishStyle, but otherwise this
+  // is only a bitwise or.
+  //
+  // We could reduce the FFI traffic we do only doing it for structs that have
+  // non-trivial FinishStyle.
+
+#define STYLE_STRUCT(name_, checkdata_cb_)                                    \
+  if (aOldContext->PeekStyle##name_()) {                                      \
+    Style##name_();                                                           \
+  }
+#include "nsStyleStructList.h"
+#undef STYLE_STRUCT
+
+#ifdef DEBUG
+  auto oldMask = aOldContext->mBits & NS_STYLE_INHERIT_MASK;
+  auto newMask = mBits & NS_STYLE_INHERIT_MASK;
+  MOZ_ASSERT((oldMask & newMask) == oldMask,
+             "Should have at least as many structs computed as the "
+             "old context!");
+#endif
+}
+
 #ifdef DEBUG
 void nsStyleContext::List(FILE* out, int32_t aIndent, bool aListDescendants)
 {
   nsAutoCString str;
   // Indent
   int32_t ix;
   for (ix = aIndent; --ix >= 0; ) {
     str.AppendLiteral("  ");
--- a/layout/style/nsStyleContext.h
+++ b/layout/style/nsStyleContext.h
@@ -382,24 +382,16 @@ public:
   #define STYLE_STRUCT(name_, checkdata_cb_)              \
     const nsStyle##name_ * PeekStyle##name_() {           \
       return DoGetStyle##name_<false>();                  \
     }
   #include "nsStyleStructList.h"
   #undef STYLE_STRUCT
 
   /**
-   * Ensures that this context's computed struct list is at least the old
-   * context's.
-   *
-   * aOldContext must not be null.
-   */
-  void EnsureStructsForServo(const nsStyleContext* aOldContext);
-
-  /**
    * Compute the style changes needed during restyling when this style
    * context is being replaced by aNewContext.  (This is nonsymmetric since
    * we optimize by skipping comparison for styles that have never been
    * requested.)
    *
    * This method returns a change hint (see nsChangeHint.h).  All change
    * hints apply to the frame and its later continuations or ib-split
    * siblings.  Most (all of those except the "NotHandledForDescendants"
@@ -431,16 +423,22 @@ private:
   template<class StyleContextLike>
   nsChangeHint CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
                                            nsChangeHint aParentHintsNotHandledForDescendants,
                                            uint32_t* aEqualStructs,
                                            uint32_t* aSamePointerStructs);
 
 public:
   /**
+   * Ensures the same structs are cached on this style context as would be
+   * done if we called aOther->CalcDifference(this).
+   */
+  void EnsureSameStructsCached(nsStyleContext* aOldContext);
+
+  /**
    * Get a color that depends on link-visitedness using this and
    * this->GetStyleIfVisited().
    *
    * @param aField A pointer to a member variable in a style struct.
    *               The member variable and its style struct must have
    *               been listed in nsCSSVisitedDependentPropList.h.
    */
   template<typename T, typename S>