Bug 1209603 patch 9 - Cache inherited style structs on the style context when we found already-cached data in the rule tree. r=heycam
authorL. David Baron <dbaron@dbaron.org>
Mon, 19 Oct 2015 20:42:29 -0700
changeset 303638 0f3e25c8e4b1a37d809d4e777edfa4d10df5cf60
parent 303637 573bb4c9a1da8d44ebb8dfed37bdf47f449139b8
child 303639 b2c58421d657b4de854528dcd1c81fda17acddc5
push id1001
push userraliiev@mozilla.com
push dateMon, 18 Jan 2016 19:06:03 +0000
treeherdermozilla-release@8b89261f3ac4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1209603, 527977, 560780
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1209603 patch 9 - Cache inherited style structs on the style context when we found already-cached data in the rule tree. r=heycam This means we obey the invariant that if we've requested an inherited struct on a context, that struct will be cached on the style context. I believe bug 527977 intended to do make us obey this invariant, but it missed the case where nsRuleNode::GetStyle* found cached data already on the rule node, and the case where nsRuleNode::WalkRuleTree found a usable struct higher in the rule tree. Without this change, patch 10 will not function correctly for inherited structs when we encounter this case, and will cause assertions in dom/base/test/test_bug560780.html due to triggering style change hints on text nodes that inherited a color struct from a parent on whose rule node the struct was stored. (It may also have caused some of the other test failures.) This should be a clear performance improvement, since the path that's being slowed down by the added work in this patch will, with the patch, now only execute once because of that work.
layout/style/nsRuleNode.cpp
layout/style/nsRuleNode.h
layout/style/nsStyleContext.cpp
layout/style/nsStyleContext.h
--- a/layout/style/nsRuleNode.cpp
+++ b/layout/style/nsRuleNode.cpp
@@ -2343,21 +2343,22 @@ nsRuleNode::WalkRuleTree(const nsStyleSt
 
   if (detail == eRuleNone && startStruct) {
     // We specified absolutely no rule information, but a parent rule in the tree
     // specified all the rule information.  We set a bit along the branch from our
     // node in the tree to the node that specified the data that tells nodes on that
     // branch that they never need to examine their rules for this particular struct type
     // ever again.
     PropagateDependentBit(aSID, ruleNode, startStruct);
-    if (isReset) {
-      // Record that we have asked for this struct on this context, but
-      // it is not cached on the context.
-      aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
-    }
+    // With this same bit set, we do two different things:
+    // For reset structs, record that we have asked for this struct on
+    // this context, but it is not cached on the context.
+    // For inherited structs, mark the struct (which will be set on
+    // the context by our caller) as not being owned by the context.
+    aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
     return startStruct;
   }
   if ((!startStruct && !isReset &&
        (detail == eRuleNone || detail == eRulePartialInherited)) ||
       detail == eRuleFullInherited) {
     // We specified no non-inherited information and neither did any of
     // our parent rules.
 
@@ -2719,18 +2720,18 @@ nsRuleNode::SetDefaultOnRoot(const nsSty
                  "Going to leak style data");                                 \
     aHighestNode->mStyleData.mInheritedData->                                 \
       mStyleStructs[eStyleStruct_##type_] = data_;                            \
     /* Propagate the bit down. */                                             \
     PropagateDependentBit(eStyleStruct_##type_, aHighestNode, data_);         \
     /* Tell the style context that it doesn't own the data */                 \
     aContext->AddStyleBit(NS_STYLE_INHERIT_BIT(type_));                       \
   }                                                                           \
-  /* Always cache inherited data on the style context */                      \
-  aContext->SetStyle##type_(data_);                                           \
+  /* For inherited structs, our caller will cache the data on the */          \
+  /* style context */                                                         \
                                                                               \
   return data_;
 
 /**
  * End an nsRuleNode::Compute*Data function for a reset struct.
  *
  * @param type_ The nsStyle* type this function computes.
  * @param data_ Variable holding the result of this function.
@@ -9408,21 +9409,22 @@ nsRuleNode::GetStyleData(nsStyleStructID
 
   const void *data;
 
   // Never use cached data for animated style inside a pseudo-element;
   // see comment on cacheability in AnimValuesStyleRule::MapRuleInfoInto.
   if (!(HasAnimationData() && ParentHasPseudoElementData(aContext))) {
     data = mStyleData.GetStyleData(aSID, aContext);
     if (MOZ_LIKELY(data != nullptr)) {
+      // With this same bit set, we do two different things:
       // For reset structs, mark the struct as having been retrieved for
       // this context.
-      if (nsCachedStyleData::IsReset(aSID)) {
-        aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
-      }
+      // For inherited structs, mark the struct (which will be set on
+      // the context by our caller) as not being owned by the context.
+      aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
       return data; // We have a fully specified struct. Just return it.
     }
   }
 
   if (MOZ_UNLIKELY(!aComputeData))
     return nullptr;
 
   // Nothing is cached.  We'll have to delve further and examine our rules.
--- a/layout/style/nsRuleNode.h
+++ b/layout/style/nsRuleNode.h
@@ -878,33 +878,40 @@ public:
                            bool aComputeData);
 
 
   // See comments in GetStyleData for an explanation of what the
   // code below does.
   #define STYLE_STRUCT_INHERITED(name_, checkdata_cb_)                        \
   template<bool aComputeData>                                                 \
   const nsStyle##name_*                                                       \
-  GetStyle##name_(nsStyleContext* aContext)                                   \
+  GetStyle##name_(nsStyleContext* aContext, uint64_t& aContextStyleBits)      \
   {                                                                           \
     NS_ASSERTION(IsUsedDirectly(),                                            \
                  "if we ever call this on rule nodes that aren't used "       \
                  "directly, we should adjust handling of mDependentBits "     \
                  "in some way.");                                             \
     MOZ_ASSERT(!ContextHasCachedData(aContext, eStyleStruct_##name_),         \
                "style context should not have cached data for struct");       \
                                                                               \
     const nsStyle##name_ *data;                                               \
                                                                               \
     /* Never use cached data for animated style inside a pseudo-element; */   \
     /* see comment on cacheability in AnimValuesStyleRule::MapRuleInfoInto */ \
     if (!(HasAnimationData() && ParentHasPseudoElementData(aContext))) {      \
       data = mStyleData.GetStyle##name_();                                    \
-      if (MOZ_LIKELY(data != nullptr))                                        \
+      if (data != nullptr) {                                                  \
+        /* For inherited structs, mark the struct (which will be set on */    \
+        /* the context by our caller) as not being owned by the context. */   \
+        /* Normally this would be aContext->AddStyleBit(), but aContext is */ \
+        /* an incomplete type here, so we work around that with a param. */   \
+        aContextStyleBits |= NS_STYLE_INHERIT_BIT(name_);                     \
+        /* Our caller will cache the data on the style context. */            \
         return data;                                                          \
+      }                                                                       \
     }                                                                         \
                                                                               \
     if (!aComputeData)                                                        \
       return nullptr;                                                         \
                                                                               \
     data = static_cast<const nsStyle##name_ *>                                \
              (WalkRuleTree(eStyleStruct_##name_, aContext));                  \
                                                                               \
--- a/layout/style/nsStyleContext.cpp
+++ b/layout/style/nsStyleContext.cpp
@@ -412,17 +412,24 @@ nsStyleContext::HasChildThatUsesGrandanc
          ListContainsStyleContextThatUsesGrandancestorStyle(mChild);
 }
 
 const void* nsStyleContext::StyleData(nsStyleStructID aSID)
 {
   const void* cachedData = GetCachedStyleData(aSID);
   if (cachedData)
     return cachedData; // We have computed data stored on this node in the context tree.
-  return mRuleNode->GetStyleData(aSID, this, true); // Our rule node will take care of it for us.
+  // Our rule node will take care of it for us.
+  const void* newData = mRuleNode->GetStyleData(aSID, this, true);
+  if (!nsCachedStyleData::IsReset(aSID)) {
+    // always cache inherited data on the style context; the rule
+    // node set the bit in mBits for us if needed.
+    mCachedInheritedData.mStyleStructs[aSID] = const_cast<void*>(newData);
+  }
+  return newData;
 }
 
 // This is an evil evil function, since it forces you to alloc your own separate copy of
 // style data!  Do not use this function unless you absolutely have to!  You should avoid
 // this at all costs! -dwh
 void* 
 nsStyleContext::GetUniqueStyleData(const nsStyleStructID& aSID)
 {
--- a/layout/style/nsStyleContext.h
+++ b/layout/style/nsStyleContext.h
@@ -530,17 +530,23 @@ private:
     const nsStyle##name_ * DoGetStyle##name_() {                        \
       const nsStyle##name_ * cachedData =                               \
         static_cast<nsStyle##name_*>(                                   \
           mCachedInheritedData.mStyleStructs[eStyleStruct_##name_]);    \
       if (cachedData) /* Have it cached already, yay */                 \
         return cachedData;                                              \
       /* Have the rulenode deal */                                      \
       AUTO_CHECK_DEPENDENCY(eStyleStruct_##name_);                      \
-      return mRuleNode->GetStyle##name_<aComputeData>(this);            \
+      const nsStyle##name_ * newData =                                  \
+        mRuleNode->GetStyle##name_<aComputeData>(this, mBits);          \
+      /* always cache inherited data on the style context; the rule */  \
+      /* node set the bit in mBits for us if needed. */                 \
+      mCachedInheritedData.mStyleStructs[eStyleStruct_##name_] =        \
+        const_cast<nsStyle##name_ *>(newData);                          \
+      return newData;                                                   \
     }
   #define STYLE_STRUCT_RESET(name_, checkdata_cb_)                      \
     template<bool aComputeData>                                         \
     const nsStyle##name_ * DoGetStyle##name_() {                        \
       if (mCachedResetData) {                                           \
         const nsStyle##name_ * cachedData =                             \
           static_cast<nsStyle##name_*>(                                 \
             mCachedResetData->mStyleStructs[eStyleStruct_##name_]);     \