Bug 1843946 - Make the MiscContainer for eAtomArray refcounted. r=emilio

This makes it cheaper to copy the nsAttrValue, which improves innerHTML times on Speedometer
because it reduces the time spent creating and destroying valueForAfterSetAttr.

It will also make it possible to reuse eAtomArray values across elements.

This patch adds a copying operation in AllocClassMatchingInfo which is needed
now that the MiscContainer's mValue.mAtomArray is immutable. This is a bit
unfortunate: In the past, this was doing one atom array allocation, and now
it's doing two; one during parsing and one during the copy. However, I would
expect that most calls to getElementsByClassName only supply a single class
name and not a set of space-separated class names, but I haven't measured it.
If this copy turns out to make things slower, we can probably avoid it by
adding a way to parse a string into an AtomArray directly without going
through nsAttrValue.

Before: https://share.firefox.dev/3DAhLzm
After: https://share.firefox.dev/456TMTM

Differential Revision: https://phabricator.services.mozilla.com/D183810

dom/base/nsAttrValue.cpp

@@ -277,6 +277,7 @@ void nsAttrValue::SetTo(const nsAttrValue& aOther) {
       cont->mValue.mColor = otherCont->mValue.mColor;
       break;
     }
+    case eAtomArray:
     case eShadowParts:
     case eCSSDeclaration: {
       MOZ_CRASH("These should be refcounted!");
@@ -285,17 +286,6 @@ void nsAttrValue::SetTo(const nsAttrValue& aOther) {
       NS_ADDREF(cont->mValue.mURL = otherCont->mValue.mURL);
       break;
     }
-    case eAtomArray: {
-      if (!EnsureEmptyAtomArray()) {
-        Reset();
-        return;
-      }
-      // XXX(Bug 1631371) Check if this should use a fallible operation as it
-      // pretended earlier.
-      *GetAtomArrayValue() = otherCont->mValue.mAtomArray->Clone();
-
-      break;
-    }
     case eDoubleValue: {
       cont->mDoubleValue = otherCont->mDoubleValue;
       break;
@@ -498,10 +488,30 @@ void nsAttrValue::RemoveDuplicatesFromAtomArray() {
     return;
   }
 
-  // We found duplicates and need to swap out our MiscContainer's mAtomArray.
-  MiscContainer* cont = GetMiscContainer();
-  delete cont->mValue.mAtomArray;
+  // We found duplicates. Wrap the new atom array into a fresh MiscContainer,
+  // and copy over the existing container's string or atom.
+
+  MiscContainer* oldCont = GetMiscContainer();
+  MOZ_ASSERT(oldCont->IsRefCounted());
+
+  uintptr_t stringBits = 0;
+  bool isString = false;
+  if (void* otherPtr = oldCont->GetStringOrAtomPtr(isString)) {
+    stringBits = oldCont->mStringBits;
+    if (isString) {
+      static_cast<nsStringBuffer*>(otherPtr)->AddRef();
+    } else {
+      static_cast<nsAtom*>(otherPtr)->AddRef();
+    }
+  }
+
+  MiscContainer* cont = EnsureEmptyMiscContainer();
+  MOZ_ASSERT(cont->mValue.mRefCount == 0);
   cont->mValue.mAtomArray = deduplicatedAtomArray.release();
+  cont->mType = eAtomArray;
+  NS_ADDREF(cont);
+  MOZ_ASSERT(cont->mValue.mRefCount == 1);
+  cont->SetStringBitsMainThread(stringBits);
 }
 
 void nsAttrValue::ToString(nsAString& aResult) const {
@@ -1180,12 +1190,12 @@ bool nsAttrValue::Contains(nsAtom* aValue,
     }
     default: {
       if (Type() == eAtomArray) {
-        AttrAtomArray* array = GetAtomArrayValue();
+        const AttrAtomArray* array = GetAtomArrayValue();
         if (aCaseSensitive == eCaseMatters) {
           return array->mArray.Contains(aValue);
         }
 
-        for (RefPtr<nsAtom>& cur : array->mArray) {
+        for (const RefPtr<nsAtom>& cur : array->mArray) {
           // For performance reasons, don't do a full on unicode case
           // insensitive string comparison. This is only used for quirks mode
           // anyway.
@@ -1214,7 +1224,7 @@ bool nsAttrValue::Contains(const nsAString& aValue) const {
     }
     default: {
       if (Type() == eAtomArray) {
-        AttrAtomArray* array = GetAtomArrayValue();
+        const AttrAtomArray* array = GetAtomArrayValue();
         return array->mArray.Contains(aValue, AtomArrayStringComparator());
       }
     }
@@ -1278,11 +1288,7 @@ void nsAttrValue::ParseAtomArray(const nsAString& aValue) {
     return;
   }
 
-  if (!EnsureEmptyAtomArray()) {
-    return;
-  }
-
-  AttrAtomArray* array = GetAtomArrayValue();
+  AttrAtomArray* array = new AttrAtomArray;
 
   // XXX(Bug 1631371) Check if this should use a fallible operation as it
   // pretended earlier.
@@ -1309,6 +1315,13 @@ void nsAttrValue::ParseAtomArray(const nsAString& aValue) {
     }
   }
 
+  MiscContainer* cont = EnsureEmptyMiscContainer();
+  MOZ_ASSERT(cont->mValue.mRefCount == 0);
+  cont->mValue.mAtomArray = array;
+  cont->mType = eAtomArray;
+  NS_ADDREF(cont);
+  MOZ_ASSERT(cont->mValue.mRefCount == 1);
+
   SetMiscAtomOrString(&aValue);
 }
 
@@ -1927,6 +1940,8 @@ MiscContainer* nsAttrValue::ClearMiscContainer() {
           break;
         }
         case eAtomArray: {
+          MOZ_ASSERT(cont->mValue.mRefCount == 1);
+          cont->Release();
           delete cont->mValue.mAtomArray;
           break;
         }
@@ -1957,20 +1972,6 @@ MiscContainer* nsAttrValue::EnsureEmptyMiscContainer() {
   return cont;
 }
 
-bool nsAttrValue::EnsureEmptyAtomArray() {
-  if (Type() == eAtomArray) {
-    ResetMiscAtomOrString();
-    GetAtomArrayValue()->Clear();
-    return true;
-  }
-
-  MiscContainer* cont = EnsureEmptyMiscContainer();
-  cont->mValue.mAtomArray = new AttrAtomArray;
-  cont->mType = eAtomArray;
-
-  return true;
-}
-
 already_AddRefed<nsStringBuffer> nsAttrValue::GetStringBuffer(
     const nsAString& aValue) const {
   uint32_t len = aValue.Length();

dom/base/nsAttrValue.h

@@ -57,13 +57,6 @@ struct AttrAtomArray {
     }
     return CreateDeduplicatedCopyIfDifferentImpl();
   }
-  AttrAtomArray Clone() const {
-    return {mArray.Clone(), mMayContainDuplicates};
-  }
-  void Clear() {
-    mArray.Clear();
-    mMayContainDuplicates = false;
-  }
   bool operator==(const AttrAtomArray& aOther) const {
     return mArray == aOther.mArray;
   }
@@ -245,7 +238,7 @@ class nsAttrValue {
   bool GetColorValue(nscolor& aColor) const;
   inline int16_t GetEnumValue() const;
   inline double GetPercentValue() const;
-  inline mozilla::AttrAtomArray* GetAtomArrayValue() const;
+  inline const mozilla::AttrAtomArray* GetAtomArrayValue() const;
   inline mozilla::DeclarationBlock* GetCSSDeclarationValue() const;
   inline nsIURI* GetURLValue() const;
   inline double GetDoubleValue() const;
@@ -536,7 +529,6 @@ class nsAttrValue {
   // Like ClearMiscContainer, except allocates a new container if one does not
   // exist already.
   MiscContainer* EnsureEmptyMiscContainer();
-  bool EnsureEmptyAtomArray();
   already_AddRefed<nsStringBuffer> GetStringBuffer(
       const nsAString& aValue) const;
   // Given an enum table and a particular entry in that table, return

dom/base/nsAttrValueInlines.h

@@ -44,7 +44,7 @@ struct MiscContainer final {
         uint32_t mEnumValue;
         mozilla::DeclarationBlock* mCSSDeclaration;
         nsIURI* mURL;
-        mozilla::AttrAtomArray* mAtomArray;
+        const mozilla::AttrAtomArray* mAtomArray;
         const mozilla::ShadowParts* mShadowParts;
         const mozilla::SVGAnimatedIntegerPair* mSVGAnimatedIntegerPair;
         const mozilla::SVGAnimatedLength* mSVGLength;
@@ -120,7 +120,8 @@ struct MiscContainer final {
     // Nothing stops us from refcounting (and sharing) other types of
     // MiscContainer (except eDoubleValue types) but there's no compelling
     // reason to.
-    return mType == nsAttrValue::eCSSDeclaration ||
+    return mType == nsAttrValue::eAtomArray ||
+           mType == nsAttrValue::eCSSDeclaration ||
            mType == nsAttrValue::eShadowParts;
   }
 
@@ -166,7 +167,7 @@ inline double nsAttrValue::GetPercentValue() const {
   return GetMiscContainer()->mDoubleValue / 100.0f;
 }
 
-inline mozilla::AttrAtomArray* nsAttrValue::GetAtomArrayValue() const {
+inline const mozilla::AttrAtomArray* nsAttrValue::GetAtomArrayValue() const {
   MOZ_ASSERT(Type() == eAtomArray, "wrong type");
   return GetMiscContainer()->mValue.mAtomArray;
 }

dom/base/nsContentUtils.cpp

@@ -6764,7 +6764,7 @@ void* nsContentUtils::AllocClassMatchingInfo(nsINode* aRootNode,
   // nsAttrValue::Equals is sensitive to order, so we'll send an array
   auto* info = new ClassMatchingInfo;
   if (attrValue.Type() == nsAttrValue::eAtomArray) {
-    info->mClasses = std::move(attrValue.GetAtomArrayValue()->mArray);
+    info->mClasses = attrValue.GetAtomArrayValue()->mArray.Clone();
   } else if (attrValue.Type() == nsAttrValue::eAtom) {
     info->mClasses.AppendElement(attrValue.GetAtomValue());
   }

servo/components/style/gecko/snapshot_helpers.rs

@@ -48,7 +48,7 @@ unsafe fn get_class_or_part_from_attr(attr: &structs::nsAttrValue) -> Class {
             structs::nsAttrValue_ValueType_eAtomArray
         );
         // NOTE: Bindgen doesn't deal with AutoTArray, so cast it below.
-        let attr_array: *mut _ = *(*container)
+        let attr_array: *const _ = *(*container)
             .__bindgen_anon_1
             .mValue
             .as_ref()