Bug 1353312 - Don't use conditions when caching a struct with no rules. r=dbaron, a=ritu FENNEC_55_0b7_BUILD1 FENNEC_55_0b7_RELEASE FIREFOX_55_0b7_BUILD1 FIREFOX_55_0b7_RELEASE
authorCameron McCormack <cam@mcc.id.au>
Wed, 05 Jul 2017 23:23:10 -0400
changeset 414190 1ae7e516e5dae3c0b537251781e1adeb50783131
parent 414189 6a8f9e8bcabd271f6c762ab49535798eaf6b84a2
child 414191 58d13a8d70b46b95179861406bcd50106ac38730
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron, ritu
bugs1353312
milestone55.0
Bug 1353312 - Don't use conditions when caching a struct with no rules. r=dbaron, a=ritu MozReview-Commit-ID: 2Q1xWcDY10T
layout/style/RuleNodeCacheConditions.h
layout/style/nsRuleNode.cpp
layout/style/nsRuleNode.h
--- a/layout/style/RuleNodeCacheConditions.h
+++ b/layout/style/RuleNodeCacheConditions.h
@@ -90,16 +90,21 @@ public:
              eHaveWritingMode;
   }
 
   void SetUncacheable()
   {
     mBits |= eUncacheable;
   }
 
+  void Clear()
+  {
+    *this = RuleNodeCacheConditions();
+  }
+
   bool Cacheable() const
   {
     return !(mBits & eUncacheable);
   }
 
   bool CacheableWithDependencies() const
   {
     return !(mBits & eUncacheable) &&
--- a/layout/style/nsRuleNode.cpp
+++ b/layout/style/nsRuleNode.cpp
@@ -2632,16 +2632,52 @@ nsRuleNode::WalkRuleTree(const nsStyleSt
   if (!highestNode)
     highestNode = rootNode;
 
   MOZ_ASSERT(!(aSID == eStyleStruct_Variables && startStruct),
              "if we start caching Variables structs in the rule tree, then "
              "not forcing detail to eRulePartialMixed just below is no "
              "longer valid");
 
+  if (detail == eRuleNone && isReset) {
+    // We specified absolutely no rule information for a reset struct, and we
+    // may or may not have found a parent rule in the tree that specified all
+    // the rule information.  Regardless, we don't need to use any cache
+    // conditions if we cache this struct in the rule tree.
+    //
+    // Normally ruleData.mConditions would already indicate that the struct
+    // is cacheable without conditions if detail is eRuleNone, but because
+    // of the UnsetPropertiesWithoutFlags call above, we may have encountered
+    // some rules with dependencies, which we then cleared out of ruleData.
+    //
+    // ruleData.mConditions could also indicate we are not cacheable at all,
+    // such as when AnimValuesStyleRule prevents us from caching structs
+    // when attempting to apply animations to pseudos.
+    //
+    // So if we we are uncacheable, we leave it, but if we are cacheable
+    // with dependencies, we convert that to cacheable without dependencies.
+    if (ruleData.mConditions.CacheableWithDependencies()) {
+      MOZ_ASSERT(pseudoRestriction,
+                 "should only be cacheable with dependencies if we had a "
+                 "pseudo restriction");
+      ruleData.mConditions.Clear();
+    } else {
+      // XXXheycam We shouldn't have `|| GetLevel() == SheetType::Transition`
+      // in the assertion condition, but rule nodes created by
+      // ResolveStyleByAddingRules don't call SetIsAnimationRule().
+      MOZ_ASSERT(ruleData.mConditions.CacheableWithoutDependencies() ||
+                 ((HasAnimationData() ||
+                   GetLevel() == SheetType::Transition) &&
+                  aContext->GetParent() &&
+                  aContext->GetParent()->HasPseudoElementData()),
+                 "should only be uncacheable if we had an animation rule "
+                 "and we're inside a pseudo");
+    }
+  }
+
   if (!ruleData.mConditions.CacheableWithoutDependencies() &&
       aSID != eStyleStruct_Variables) {
     // Treat as though some data is specified to avoid the optimizations and
     // force data computation.
     //
     // We don't need to do this for Variables structs since we know those are
     // never cached in the rule tree, and it avoids wasteful computation of a
     // new Variables struct when we have no additional variable declarations,
--- a/layout/style/nsRuleNode.h
+++ b/layout/style/nsRuleNode.h
@@ -185,30 +185,35 @@ private:
   void* GetConditionalStyleData(nsStyleStructID aSID,
                                 nsStyleContext* aStyleContext) const;
 
 public:
   void SetStyleData(nsStyleStructID aSID, void* aStyleStruct) {
     MOZ_ASSERT(!(mConditionalBits & GetBitForSID(aSID)),
                "rule node should not have unconditional and conditional style "
                "data for a given struct");
+    mConditionalBits &= ~GetBitForSID(aSID);
     mEntries[aSID] = aStyleStruct;
   }
 
   void SetStyleData(nsStyleStructID aSID,
                     nsPresContext* aPresContext,
                     void* aStyleStruct,
                     const mozilla::RuleNodeCacheConditions& aConditions) {
-    MOZ_ASSERT((mConditionalBits & GetBitForSID(aSID)) ||
-               !mEntries[aSID],
-               "rule node should not have unconditional and conditional style "
-               "data for a given struct");
+    if (!(mConditionalBits & GetBitForSID(aSID))) {
+      MOZ_ASSERT(!mEntries[aSID],
+                 "rule node should not have unconditional and conditional "
+                 "style data for a given struct");
+      mEntries[aSID] = nullptr;
+    }
+
     MOZ_ASSERT(aConditions.CacheableWithDependencies(),
                "don't call SetStyleData with a cache key that has no "
                "conditions or is uncacheable");
+
 #ifdef DEBUG
     for (Entry* e = static_cast<Entry*>(mEntries[aSID]); e; e = e->mNext) {
       NS_WARNING_ASSERTION(e->mConditions != aConditions,
                            "wasteful to have duplicate conditional style data");
     }
 #endif
 
     mConditionalBits |= GetBitForSID(aSID);