Bug 1209603 patch 8 - Record in mBits when we have gotten a reset style struct that is cached on the rule node. r=heycam
authorL. David Baron <dbaron@dbaron.org>
Mon, 19 Oct 2015 20:42:29 -0700
changeset 301862 573bb4c9a1da8d44ebb8dfed37bdf47f449139b8
parent 301861 514b6bfc3f3896ee44a5c75698c10ff94a6855b1
child 301863 0f3e25c8e4b1a37d809d4e777edfa4d10df5cf60
push id5392
push userraliiev@mozilla.com
push dateMon, 14 Dec 2015 20:08:23 +0000
treeherdermozilla-beta@16ce8562a975 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1209603
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 8 - Record in mBits when we have gotten a reset style struct that is cached on the rule node. r=heycam I'm a little worried about the performance of the change to nsRuleNode::GetStyle*, which sets a bit on the style context every time a struct getter goes through it. It's not obvious how that compares to the performance benefit from patch 10.
layout/style/nsRuleNode.cpp
layout/style/nsRuleNode.h
layout/style/nsStyleContext.h
--- a/layout/style/nsRuleNode.cpp
+++ b/layout/style/nsRuleNode.cpp
@@ -2343,16 +2343,21 @@ 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));
+    }
     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.
 
@@ -2712,18 +2717,17 @@ nsRuleNode::SetDefaultOnRoot(const nsSty
     NS_ASSERTION(!aHighestNode->mStyleData.mInheritedData->                   \
                    mStyleStructs[eStyleStruct_##type_],                       \
                  "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(nsCachedStyleData::GetBitForSID(eStyleStruct_##type_));     \
+    aContext->AddStyleBit(NS_STYLE_INHERIT_BIT(type_));                       \
   }                                                                           \
   /* Always cache inherited data on the style context */                      \
   aContext->SetStyle##type_(data_);                                           \
                                                                               \
   return data_;
 
 /**
  * End an nsRuleNode::Compute*Data function for a reset struct.
@@ -2748,25 +2752,27 @@ nsRuleNode::SetDefaultOnRoot(const nsSty
     }                                                                         \
     NS_ASSERTION(!aHighestNode->mStyleData.mResetData->                       \
                    GetStyleData(eStyleStruct_##type_),                        \
                  "Going to leak style data");                                 \
     aHighestNode->mStyleData.mResetData->                                     \
       SetStyleData(eStyleStruct_##type_, data_);                              \
     /* Propagate the bit down. */                                             \
     PropagateDependentBit(eStyleStruct_##type_, aHighestNode, data_);         \
+    /* Tell the context that we've gotten the data (separate meaning */       \
+    /* of mBits when the cached data pointer is null) */                      \
+    aContext->AddStyleBit(NS_STYLE_INHERIT_BIT(type_));                       \
   } else if (conditions.Cacheable()) {                                        \
     if (!mStyleData.mResetData) {                                             \
       mStyleData.mResetData = new (mPresContext) nsConditionalResetStyleData; \
     }                                                                         \
     mStyleData.mResetData->                                                   \
       SetStyleData(eStyleStruct_##type_, mPresContext, data_, conditions);    \
     /* Tell the style context that it doesn't own the data */                 \
-    aContext->                                                                \
-      AddStyleBit(nsCachedStyleData::GetBitForSID(eStyleStruct_##type_));     \
+    aContext->AddStyleBit(NS_STYLE_INHERIT_BIT(type_));                       \
     aContext->SetStyle(eStyleStruct_##type_, data_);                          \
   } else {                                                                    \
     /* We can't be cached in the rule node.  We have to be put right */       \
     /* on the style context. */                                               \
     aContext->SetStyle(eStyleStruct_##type_, data_);                          \
     if (aContext->GetParent()) {                                              \
       /* This is pessimistic; we could be uncacheable because we had a */     \
       /* relative font-weight, for example, which does not need to defeat */  \
@@ -9401,18 +9407,24 @@ nsRuleNode::GetStyleData(nsStyleStructID
              "style context should not have cached data for struct");
 
   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))
+    if (MOZ_LIKELY(data != nullptr)) {
+      // For reset structs, mark the struct as having been retrieved for
+      // this context.
+      if (nsCachedStyleData::IsReset(aSID)) {
+        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.
   data = WalkRuleTree(aSID, aContext);
 
--- a/layout/style/nsRuleNode.h
+++ b/layout/style/nsRuleNode.h
@@ -910,33 +910,38 @@ public:
                                                                               \
     MOZ_ASSERT(data, "should have aborted on out-of-memory");                 \
     return data;                                                              \
   }
 
   #define STYLE_STRUCT_RESET(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_(aContext);                            \
-      if (MOZ_LIKELY(data != nullptr))                                        \
+      if (MOZ_LIKELY(data != nullptr)) {                                      \
+        /* Mark the struct as having been retrieved for this 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_);                     \
         return data;                                                          \
+      }                                                                       \
     }                                                                         \
                                                                               \
     if (!aComputeData)                                                        \
       return nullptr;                                                         \
                                                                               \
     data = static_cast<const nsStyle##name_ *>                                \
              (WalkRuleTree(eStyleStruct_##name_, aContext));                  \
                                                                               \
--- a/layout/style/nsStyleContext.h
+++ b/layout/style/nsStyleContext.h
@@ -544,17 +544,17 @@ private:
         const nsStyle##name_ * cachedData =                             \
           static_cast<nsStyle##name_*>(                                 \
             mCachedResetData->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);            \
+      return mRuleNode->GetStyle##name_<aComputeData>(this, mBits);     \
     }
   #include "nsStyleStructList.h"
   #undef STYLE_STRUCT_RESET
   #undef STYLE_STRUCT_INHERITED
 
   // Helper for ClearCachedInheritedStyleDataOnDescendants.
   void DoClearCachedInheritedStyleDataOnDescendants(uint32_t aStructs);
 
@@ -614,16 +614,20 @@ private:
   // mCachedResetData.
   nsResetStyleData*       mCachedResetData; // Cached reset style data.
   nsInheritedStyleData    mCachedInheritedData; // Cached inherited style data
 
   // mBits stores a number of things:
   //  - For all structs, when they are non-null in the style context's
   //    storage, it records (using the style struct bits) which structs
   //    are inherited from the parent context or owned by mRuleNode.
+  //  - For reset (non-inherited) style structs, when they are null in
+  //    the style context's storage, it records that we have been asked
+  //    for that struct.  (We don't need this for inherited structs
+  //    since we always cache them in mCachedInheritedData.)
   //  - It also stores the additional bits listed at the top of
   //    nsStyleStruct.h.
   uint64_t                mBits;
 
   uint32_t                mRefCnt;
 
 #ifdef DEBUG
   uint32_t                mFrameRefCnt; // number of frames that use this