Bug 978833 patch 7 - Fuse allocation of ImportantStyleData with Declaration. r=heycam
authorL. David Baron <dbaron@dbaron.org>
Thu, 05 Nov 2015 16:44:09 +0800
changeset 271213 4b633979383a8174c450ed7f77c0f06b6095f404
parent 271212 e62f0b7f0a02d3d6b7be90cef29075a56894738d
child 271214 0adcd9f3fac0358d859575727b3febeaf420c512
push id67603
push userdbaron@mozilla.com
push dateThu, 05 Nov 2015 08:44:59 +0000
treeherdermozilla-inbound@cf480f83f25d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs978833
milestone45.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 978833 patch 7 - Fuse allocation of ImportantStyleData with Declaration. r=heycam Note that this adds a new public API to css::Declaration; the equivalent API is removed from css::StyleRule and nsCSSPageRule in patch 13. But the removal and addition need to be on opposite sides of patch 12. This fused allocation is no larger than having a pointer, and it removes having to worry about cycles.
layout/style/Declaration.cpp
layout/style/Declaration.h
layout/style/StyleRule.cpp
layout/style/StyleRule.h
layout/style/nsCSSRules.cpp
layout/style/nsCSSRules.h
--- a/layout/style/Declaration.cpp
+++ b/layout/style/Declaration.cpp
@@ -14,31 +14,24 @@
 #include "mozilla/css/Declaration.h"
 #include "nsPrintfCString.h"
 #include "gfxFontConstants.h"
 #include "nsStyleUtil.h"
 
 namespace mozilla {
 namespace css {
 
-ImportantStyleData::ImportantStyleData(Declaration* aDeclaration)
-  : mDeclaration(aDeclaration)
-{
-}
-
-ImportantStyleData::~ImportantStyleData()
-{
-}
-
-NS_IMPL_ISUPPORTS(ImportantStyleData, nsIStyleRule)
+NS_IMPL_QUERY_INTERFACE(ImportantStyleData, nsIStyleRule)
+NS_IMPL_ADDREF_USING_AGGREGATOR(ImportantStyleData, Declaration())
+NS_IMPL_RELEASE_USING_AGGREGATOR(ImportantStyleData, Declaration())
 
 /* virtual */ void
 ImportantStyleData::MapRuleInfoInto(nsRuleData* aRuleData)
 {
-  mDeclaration->MapImportantRuleInfoInto(aRuleData);
+  Declaration()->MapImportantRuleInfoInto(aRuleData);
 }
 
 #ifdef DEBUG
 /* virtual */ void
 ImportantStyleData::List(FILE* out, int32_t aIndent) const
 {
   // Indent
   nsAutoCString str;
--- a/layout/style/Declaration.h
+++ b/layout/style/Declaration.h
@@ -34,33 +34,43 @@
   { 0x90, 0xd5, 0xcc, 0x93, 0xf8, 0x53, 0xe0, 0x48 } }
 
 namespace mozilla {
 namespace css {
 
 class Rule;
 class Declaration;
 
+/**
+ * ImportantStyleData is the implementation of nsIStyleRule (a source of
+ * style data) representing the style data coming from !important rules;
+ * the !important declarations need a separate nsIStyleRule object since
+ * they fit at a different point in the cascade.
+ *
+ * ImportantStyleData is allocated only as part of a Declaration object.
+ */
 class ImportantStyleData final : public nsIStyleRule
 {
 public:
-  explicit ImportantStyleData(Declaration* aDeclaration);
 
   NS_DECL_ISUPPORTS
 
+  inline ::mozilla::css::Declaration* Declaration();
+
   // nsIStyleRule interface
   virtual void MapRuleInfoInto(nsRuleData* aRuleData) override;
 #ifdef DEBUG
   virtual void List(FILE* out = stdout, int32_t aIndent = 0) const override;
 #endif
 
-protected:
-  virtual ~ImportantStyleData();
+private:
+  ImportantStyleData() {}
+  ~ImportantStyleData() {}
 
-  RefPtr<Declaration> mDeclaration;
+  friend class ::mozilla::css::Declaration;
 };
 
 // Declaration objects have unusual lifetime rules.  Every declaration
 // begins life in an invalid state which ends when InitializeEmpty or
 // CompressFrom is called upon it.  After that, it can be attached to
 // exactly one style rule, and will be destroyed when that style rule
 // is destroyed.  A declaration becomes immutable when its style rule's
 // |RuleMatched| method is called; after that, it must be copied before
@@ -305,16 +315,23 @@ public:
   void SetOwningRule(Rule* aRule) {
     MOZ_ASSERT(!mOwningRule || !aRule,
                "should never overwrite one rule with another");
     mOwningRule = aRule;
   }
 
   Rule* GetOwningRule() { return mOwningRule; }
 
+  ImportantStyleData* GetImportantStyleData() {
+    if (HasImportantData()) {
+      return &mImportantStyleData;
+    }
+    return nullptr;
+  }
+
 private:
   Declaration& operator=(const Declaration& aCopy) = delete;
   bool operator==(const Declaration& aCopy) const = delete;
 
   void GetValue(nsCSSProperty aProperty, nsAString& aValue,
                 nsCSSValue::Serialization aValueSerialization) const;
 
   static void AppendImportanceToString(bool aIsImportant, nsAString& aString);
@@ -383,19 +400,35 @@ private:
   nsAutoPtr<CSSVariableDeclarations> mVariables;
 
   // may be null
   nsAutoPtr<CSSVariableDeclarations> mImportantVariables;
 
   // The style rule that owns this declaration.  May be null.
   Rule* mOwningRule;
 
+  friend class ImportantStyleData;
+  ImportantStyleData mImportantStyleData;
+
   // set by style rules when |RuleMatched| is called;
   // also by ToString (hence the 'mutable').
   mutable bool mImmutable;
 };
 
+inline ::mozilla::css::Declaration*
+ImportantStyleData::Declaration()
+{
+  union {
+    char* ch; /* for pointer arithmetic */
+    ::mozilla::css::Declaration* declaration;
+    ImportantStyleData* importantData;
+  } u;
+  u.importantData = this;
+  u.ch -= offsetof(::mozilla::css::Declaration, mImportantStyleData);
+  return u.declaration;
+}
+
 NS_DEFINE_STATIC_IID_ACCESSOR(Declaration, NS_CSS_DECLARATION_IMPL_CID)
 
 } // namespace css
 } // namespace mozilla
 
 #endif /* mozilla_css_Declaration_h */
--- a/layout/style/StyleRule.cpp
+++ b/layout/style/StyleRule.cpp
@@ -1447,23 +1447,18 @@ NS_INTERFACE_MAP_END
 
 NS_IMPL_ADDREF(StyleRule)
 NS_IMPL_RELEASE(StyleRule)
 
 void
 StyleRule::RuleMatched()
 {
   if (!mWasMatched) {
-    MOZ_ASSERT(!mImportantRule, "should not have important rule yet");
-
     mWasMatched = true;
     mDeclaration->SetImmutable();
-    if (mDeclaration->HasImportantData()) {
-      mImportantRule = new ImportantStyleData(mDeclaration);
-    }
   }
 }
 
 /* virtual */ int32_t
 StyleRule::GetType() const
 {
   return Rule::STYLE_RULE;
 }
@@ -1617,17 +1612,16 @@ StyleRule::SetSelectorText(const nsAStri
 StyleRule::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
 {
   size_t n = aMallocSizeOf(this);
   n += mSelector ? mSelector->SizeOfIncludingThis(aMallocSizeOf) : 0;
   n += mDeclaration ? mDeclaration->SizeOfIncludingThis(aMallocSizeOf) : 0;
 
   // Measurement of the following members may be added later if DMD finds it is
   // worthwhile:
-  // - mImportantRule;
   // - mDOMRule;
 
   return n;
 }
 
 
 } // namespace css
 } // namespace mozilla
--- a/layout/style/StyleRule.h
+++ b/layout/style/StyleRule.h
@@ -321,17 +321,19 @@ public:
    * the declaration is modified.
    *
    * |DeclarationChanged| handles replacing the object in the container
    * sheet or group rule if |aHandleContainer| is true.
    */
   already_AddRefed<StyleRule>
   DeclarationChanged(Declaration* aDecl, bool aHandleContainer);
 
-  nsIStyleRule* GetImportantRule() const { return mImportantRule; }
+  nsIStyleRule* GetImportantRule() const {
+    return mDeclaration->GetImportantStyleData();
+  }
 
   /**
    * The rule processor must call this method before calling
    * nsRuleWalker::Forward on this rule during rule matching.
    */
   void RuleMatched();
 
   // hooks for DOM rule
@@ -358,17 +360,16 @@ public:
   virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override;
 
 private:
   ~StyleRule();
 
 private:
   nsCSSSelectorList*      mSelector; // null for style attribute
   RefPtr<Declaration>     mDeclaration;
-  RefPtr<ImportantStyleData> mImportantRule; // initialized by RuleMatched
   RefPtr<DOMCSSStyleRule> mDOMRule;
 
 private:
   StyleRule& operator=(const StyleRule& aCopy) = delete;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(StyleRule, NS_CSS_STYLE_RULE_IMPL_CID)
 
--- a/layout/style/nsCSSRules.cpp
+++ b/layout/style/nsCSSRules.cpp
@@ -2696,28 +2696,16 @@ nsCSSPageRule::GetParentRule(nsIDOMCSSRu
 }
 
 css::Rule*
 nsCSSPageRule::GetCSSRule()
 {
   return Rule::GetCSSRule();
 }
 
-css::ImportantStyleData*
-nsCSSPageRule::GetImportantRule()
-{
-  if (!mDeclaration->HasImportantData()) {
-    return nullptr;
-  }
-  if (!mImportantRule) {
-    mImportantRule = new css::ImportantStyleData(mDeclaration);
-  }
-  return mImportantRule;
-}
-
 /* virtual */ void
 nsCSSPageRule::MapRuleInfoInto(nsRuleData* aRuleData)
 {
   mDeclaration->MapRuleInfoInto(aRuleData);
 }
 
 NS_IMETHODIMP
 nsCSSPageRule::GetStyle(nsIDOMCSSStyleDeclaration** aStyle)
@@ -2727,17 +2715,16 @@ nsCSSPageRule::GetStyle(nsIDOMCSSStyleDe
   }
   NS_ADDREF(*aStyle = mDOMDeclaration);
   return NS_OK;
 }
 
 void
 nsCSSPageRule::ChangeDeclaration(css::Declaration* aDeclaration)
 {
-  mImportantRule = nullptr;
   if (aDeclaration != mDeclaration) {
     mDeclaration->SetOwningRule(nullptr);
     mDeclaration = aDeclaration;
     mDeclaration->SetOwningRule(this);
   }
 
   CSSStyleSheet* sheet = GetStyleSheet();
   if (sheet) {
--- a/layout/style/nsCSSRules.h
+++ b/layout/style/nsCSSRules.h
@@ -521,17 +521,16 @@ protected:
 class nsCSSPageRule final : public mozilla::css::Rule,
                             public nsIDOMCSSPageRule
 {
 public:
   nsCSSPageRule(mozilla::css::Declaration* aDeclaration,
                 uint32_t aLineNumber, uint32_t aColumnNumber)
     : mozilla::css::Rule(aLineNumber, aColumnNumber)
     , mDeclaration(aDeclaration)
-    , mImportantRule(nullptr)
   {
     mDeclaration->SetOwningRule(this);
   }
 private:
   nsCSSPageRule(const nsCSSPageRule& aCopy);
   ~nsCSSPageRule();
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
@@ -552,24 +551,25 @@ public:
 
   // nsIDOMCSSPageRule interface
   NS_DECL_NSIDOMCSSPAGERULE
 
   mozilla::css::Declaration* Declaration()   { return mDeclaration; }
 
   void ChangeDeclaration(mozilla::css::Declaration* aDeclaration);
 
-  mozilla::css::ImportantStyleData* GetImportantRule();
+  mozilla::css::ImportantStyleData* GetImportantRule() {
+    return mDeclaration->GetImportantStyleData();
+  }
 
   virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override;
 private:
   RefPtr<mozilla::css::Declaration>     mDeclaration;
   // lazily created when needed:
   RefPtr<nsCSSPageStyleDeclaration>     mDOMDeclaration;
-  RefPtr<mozilla::css::ImportantStyleData> mImportantRule;
 };
 
 namespace mozilla {
 
 class CSSSupportsRule : public css::GroupRule,
                         public nsIDOMCSSSupportsRule
 {
 public: