Bug 1482866: Simplify AttrArray's implementation. r=smaug

No need for void* stuff, but I had to keep the semi-manual memory
management to allocate everything inline as we currently do.

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

dom/base/AttrArray.cpp

@@ -24,62 +24,33 @@
 
 using mozilla::CheckedUint32;
 
-/**
- * Due to a compiler bug in VisualAge C++ for AIX, we need to return the
- * address of the first index into mBuffer here, instead of simply returning
- * mBuffer itself.
- *
- * See Bug 231104 for more information.
- */
-#define ATTRS(_impl) \
-  reinterpret_cast<InternalAttr*>(&((_impl)->mBuffer[0]))
-
-
-#define NS_IMPL_EXTRA_SIZE \
-  ((sizeof(Impl) - sizeof(mImpl->mBuffer)) / sizeof(void*))
-
-AttrArray::AttrArray()
-  : mImpl(nullptr)
+AttrArray::Impl::~Impl()
 {
-}
-
-AttrArray::~AttrArray()
-{
-  if (!mImpl) {
-    return;
+  for (InternalAttr& attr : NonMappedAttrs()) {
+    attr.~InternalAttr();
   }
 
-  Clear();
-
-  free(mImpl);
-}
-
-uint32_t
-AttrArray::AttrCount() const
-{
-  return NonMappedAttrCount() + MappedAttrCount();
+  NS_IF_RELEASE(mMappedAttrs);
 }
 
 const nsAttrValue*
 AttrArray::GetAttr(nsAtom* aLocalName, int32_t aNamespaceID) const
 {
-  uint32_t i, slotCount = AttrSlotCount();
   if (aNamespaceID == kNameSpaceID_None) {
     // This should be the common case so lets make an optimized loop
-    for (i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-      if (ATTRS(mImpl)[i].mName.Equals(aLocalName)) {
-        return &ATTRS(mImpl)[i].mValue;
+    for (const InternalAttr& attr : NonMappedAttrs()) {
+      if (attr.mName.Equals(aLocalName)) {
+        return &attr.mValue;
       }
     }
 
     if (mImpl && mImpl->mMappedAttrs) {
       return mImpl->mMappedAttrs->GetAttr(aLocalName);
     }
-  }
-  else {
-    for (i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-      if (ATTRS(mImpl)[i].mName.Equals(aLocalName, aNamespaceID)) {
-        return &ATTRS(mImpl)[i].mValue;
+  } else {
+    for (const InternalAttr& attr : NonMappedAttrs()) {
+      if (attr.mName.Equals(aLocalName, aNamespaceID)) {
+        return &attr.mValue;
       }
     }
   }
@@ -90,10 +61,9 @@ AttrArray::GetAttr(nsAtom* aLocalName, int32_t aNamespaceID) const
 const nsAttrValue*
 AttrArray::GetAttr(const nsAString& aLocalName) const
 {
-  uint32_t i, slotCount = AttrSlotCount();
-  for (i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-    if (ATTRS(mImpl)[i].mName.Equals(aLocalName)) {
-      return &ATTRS(mImpl)[i].mValue;
+  for (const InternalAttr& attr : NonMappedAttrs()) {
+    if (attr.mName.Equals(aLocalName)) {
+      return &attr.mValue;
     }
   }
 
@@ -119,19 +89,14 @@ AttrArray::GetAttr(const nsAString& aName,
     return GetAttr(lowercase, eCaseMatters);
   }
 
-  uint32_t i, slotCount = AttrSlotCount();
-  for (i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-    if (ATTRS(mImpl)[i].mName.QualifiedNameEquals(aName)) {
-      return &ATTRS(mImpl)[i].mValue;
+  for (const InternalAttr& attr : NonMappedAttrs()) {
+    if (attr.mName.QualifiedNameEquals(aName)) {
+      return &attr.mValue;
     }
   }
 
   if (mImpl && mImpl->mMappedAttrs) {
-    const nsAttrValue* val =
-      mImpl->mMappedAttrs->GetAttr(aName);
-    if (val) {
-      return val;
-    }
+    return mImpl->mMappedAttrs->GetAttr(aName);
   }
 
   return nullptr;
@@ -145,35 +110,45 @@ AttrArray::AttrAt(uint32_t aPos) const
 
   uint32_t nonmapped = NonMappedAttrCount();
   if (aPos < nonmapped) {
-    return &ATTRS(mImpl)[aPos].mValue;
+    return &mImpl->NonMappedAttrs()[aPos].mValue;
   }
 
   return mImpl->mMappedAttrs->AttrAt(aPos - nonmapped);
 }
 
+template<typename Name>
+inline nsresult
+AttrArray::AddNewAttribute(Name* aName, nsAttrValue& aValue)
+{
+  MOZ_ASSERT(!mImpl || mImpl->mCapacity >= mImpl->mAttrCount);
+  if (!mImpl || mImpl->mCapacity == mImpl->mAttrCount) {
+    if (!GrowBy(1)) {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
+  }
+
+  InternalAttr& attr = mImpl->mBuffer[mImpl->mAttrCount++];
+  new (&attr.mName) nsAttrName(aName);
+  new (&attr.mValue) nsAttrValue();
+  attr.mValue.SwapValueWith(aValue);
+  return NS_OK;
+}
+
 nsresult
 AttrArray::SetAndSwapAttr(nsAtom* aLocalName, nsAttrValue& aValue,
                           bool* aHadValue)
 {
   *aHadValue = false;
-  uint32_t i, slotCount = AttrSlotCount();
-  for (i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-    if (ATTRS(mImpl)[i].mName.Equals(aLocalName)) {
-      ATTRS(mImpl)[i].mValue.SwapValueWith(aValue);
+
+  for (InternalAttr& attr : NonMappedAttrs()) {
+    if (attr.mName.Equals(aLocalName)) {
+      attr.mValue.SwapValueWith(aValue);
       *aHadValue = true;
       return NS_OK;
     }
   }
 
-  if (i == slotCount && !AddAttrSlot()) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  new (&ATTRS(mImpl)[i].mName) nsAttrName(aLocalName);
-  new (&ATTRS(mImpl)[i].mValue) nsAttrValue();
-  ATTRS(mImpl)[i].mValue.SwapValueWith(aValue);
-
-  return NS_OK;
+  return AddNewAttribute(aLocalName, aValue);
 }
 
 nsresult
@@ -187,25 +162,16 @@ AttrArray::SetAndSwapAttr(mozilla::dom::NodeInfo* aName,
   }
 
   *aHadValue = false;
-  uint32_t i, slotCount = AttrSlotCount();
-  for (i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-    if (ATTRS(mImpl)[i].mName.Equals(localName, namespaceID)) {
-      ATTRS(mImpl)[i].mName.SetTo(aName);
-      ATTRS(mImpl)[i].mValue.SwapValueWith(aValue);
+  for (InternalAttr& attr : NonMappedAttrs()) {
+    if (attr.mName.Equals(localName, namespaceID)) {
+      attr.mName.SetTo(aName);
+      attr.mValue.SwapValueWith(aValue);
       *aHadValue = true;
       return NS_OK;
     }
   }
 
-  if (i == slotCount && !AddAttrSlot()) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  new (&ATTRS(mImpl)[i].mName) nsAttrName(aName);
-  new (&ATTRS(mImpl)[i].mValue) nsAttrValue();
-  ATTRS(mImpl)[i].mValue.SwapValueWith(aValue);
-
-  return NS_OK;
+  return AddNewAttribute(aName, aValue);
 }
 
 nsresult
@@ -215,14 +181,14 @@ AttrArray::RemoveAttrAt(uint32_t aPos, nsAttrValue& aValue)
 
   uint32_t nonmapped = NonMappedAttrCount();
   if (aPos < nonmapped) {
-    ATTRS(mImpl)[aPos].mValue.SwapValueWith(aValue);
-    ATTRS(mImpl)[aPos].~InternalAttr();
+    mImpl->mBuffer[aPos].mValue.SwapValueWith(aValue);
+    mImpl->mBuffer[aPos].~InternalAttr();
 
-    uint32_t slotCount = AttrSlotCount();
-    memmove(&ATTRS(mImpl)[aPos],
-            &ATTRS(mImpl)[aPos + 1],
-            (slotCount - aPos - 1) * sizeof(InternalAttr));
-    memset(&ATTRS(mImpl)[slotCount - 1], 0, sizeof(InternalAttr));
+    memmove(mImpl->mBuffer + aPos,
+            mImpl->mBuffer + aPos + 1,
+            (mImpl->mAttrCount - aPos - 1) * sizeof(InternalAttr));
+
+    --mImpl->mAttrCount;
 
     return NS_OK;
   }
@@ -252,11 +218,13 @@ AttrArray::AttrInfoAt(uint32_t aPos) const
 
   uint32_t nonmapped = NonMappedAttrCount();
   if (aPos < nonmapped) {
-    return BorrowedAttrInfo(&ATTRS(mImpl)[aPos].mName, &ATTRS(mImpl)[aPos].mValue);
+    InternalAttr& attr = mImpl->mBuffer[aPos];
+    return BorrowedAttrInfo(&attr.mName, &attr.mValue);
   }
 
-  return BorrowedAttrInfo(mImpl->mMappedAttrs->NameAt(aPos - nonmapped),
-                    mImpl->mMappedAttrs->AttrAt(aPos - nonmapped));
+  return BorrowedAttrInfo(
+      mImpl->mMappedAttrs->NameAt(aPos - nonmapped),
+      mImpl->mMappedAttrs->AttrAt(aPos - nonmapped));
 }
 
 const nsAttrName*
@@ -267,7 +235,7 @@ AttrArray::AttrNameAt(uint32_t aPos) const
 
   uint32_t nonmapped = NonMappedAttrCount();
   if (aPos < nonmapped) {
-    return &ATTRS(mImpl)[aPos].mName;
+    return &mImpl->mBuffer[aPos].mName;
   }
 
   return mImpl->mMappedAttrs->NameAt(aPos - nonmapped);
@@ -278,12 +246,7 @@ AttrArray::GetSafeAttrNameAt(uint32_t aPos) const
 {
   uint32_t nonmapped = NonMappedAttrCount();
   if (aPos < nonmapped) {
-    void** pos = mImpl->mBuffer + aPos * ATTRSIZE;
-    if (!*pos) {
-      return nullptr;
-    }
-
-    return &reinterpret_cast<InternalAttr*>(pos)->mName;
+    return &mImpl->mBuffer[aPos].mName;
   }
 
   if (aPos >= AttrCount()) {
@@ -296,10 +259,9 @@ AttrArray::GetSafeAttrNameAt(uint32_t aPos) const
 const nsAttrName*
 AttrArray::GetExistingAttrNameFromQName(const nsAString& aName) const
 {
-  uint32_t i, slotCount = AttrSlotCount();
-  for (i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-    if (ATTRS(mImpl)[i].mName.QualifiedNameEquals(aName)) {
-      return &ATTRS(mImpl)[i].mName;
+  for (const InternalAttr& attr : NonMappedAttrs()) {
+    if (attr.mName.QualifiedNameEquals(aName)) {
+      return &attr.mName;
     }
   }
 
@@ -313,16 +275,19 @@ AttrArray::GetExistingAttrNameFromQName(const nsAString& aName) const
 int32_t
 AttrArray::IndexOfAttr(nsAtom* aLocalName, int32_t aNamespaceID) const
 {
+  if (!mImpl) {
+    return -1;
+  }
+
   int32_t idx;
-  if (mImpl && mImpl->mMappedAttrs && aNamespaceID == kNameSpaceID_None) {
+  if (mImpl->mMappedAttrs && aNamespaceID == kNameSpaceID_None) {
     idx = mImpl->mMappedAttrs->IndexOfAttr(aLocalName);
     if (idx >= 0) {
       return NonMappedAttrCount() + idx;
     }
   }
 
-  uint32_t i;
-  uint32_t slotCount = AttrSlotCount();
+  uint32_t i = 0;
   if (aNamespaceID == kNameSpaceID_None) {
     // This should be the common case so lets make an optimized loop
     // Note that here we don't check for AttrSlotIsTaken() in the loop
@@ -330,18 +295,18 @@ AttrArray::IndexOfAttr(nsAtom* aLocalName, int32_t aNamespaceID) const
     // against null would fail in the loop body (since Equals() just compares
     // the raw pointer value of aLocalName to what AttrSlotIsTaken() would be
     // checking.
-    for (i = 0; i < slotCount; ++i) {
-      if (ATTRS(mImpl)[i].mName.Equals(aLocalName)) {
-        MOZ_ASSERT(AttrSlotIsTaken(i), "sanity check");
+    for (const InternalAttr& attr : NonMappedAttrs()) {
+      if (attr.mName.Equals(aLocalName)) {
         return i;
       }
+      ++i;
     }
-  }
-  else {
-    for (i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-      if (ATTRS(mImpl)[i].mName.Equals(aLocalName, aNamespaceID)) {
+  } else {
+    for (const InternalAttr& attr : NonMappedAttrs()) {
+      if (attr.mName.Equals(aLocalName, aNamespaceID)) {
         return i;
       }
+      ++i;
     }
   }
 
@@ -406,66 +371,29 @@ AttrArray::Compact()
     return;
   }
 
-  // First compress away empty attrslots
-  uint32_t slotCount = AttrSlotCount();
-  uint32_t attrCount = NonMappedAttrCount();
-
-  if (attrCount < slotCount) {
-    SetAttrSlotCount(attrCount);
-  }
-
-  // Then resize or free buffer
-  uint32_t newSize = attrCount * ATTRSIZE;
-  if (!newSize && !mImpl->mMappedAttrs) {
-    free(mImpl);
-    mImpl = nullptr;
-  }
-  else if (newSize < mImpl->mBufferSize) {
-    mImpl = static_cast<Impl*>(realloc(mImpl, (newSize + NS_IMPL_EXTRA_SIZE) * sizeof(nsIContent*)));
-    NS_ASSERTION(mImpl, "failed to reallocate to smaller buffer");
-
-    mImpl->mBufferSize = newSize;
-  }
-}
-
-void
-AttrArray::Clear()
-{
-  if (!mImpl) {
+  if (!mImpl->mAttrCount && !mImpl->mMappedAttrs) {
+    mImpl.reset();
     return;
   }
 
-  if (mImpl->mMappedAttrs) {
-    NS_RELEASE(mImpl->mMappedAttrs);
+  // Nothing to do.
+  if (mImpl->mAttrCount == mImpl->mCapacity) {
+    return;
   }
 
-  uint32_t i, slotCount = AttrSlotCount();
-  for (i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-    ATTRS(mImpl)[i].~InternalAttr();
-  }
-
-  SetAttrSlotCount(0);
+  Impl* impl = mImpl.release();
+  impl = static_cast<Impl*>(
+    realloc(impl, Impl::AllocationSizeForAttributes(impl->mAttrCount)));
+  MOZ_ASSERT(impl, "failed to reallocate to a smaller buffer!");
+  impl->mCapacity = impl->mAttrCount;
+  mImpl.reset(impl);
 }
 
 uint32_t
-AttrArray::NonMappedAttrCount() const
+AttrArray::DoGetMappedAttrCount() const
 {
-  if (!mImpl) {
-    return 0;
-  }
-
-  uint32_t count = AttrSlotCount();
-  while (count > 0 && !mImpl->mBuffer[(count - 1) * ATTRSIZE]) {
-    --count;
-  }
-
-  return count;
-}
-
-uint32_t
-AttrArray::MappedAttrCount() const
-{
-  return mImpl && mImpl->mMappedAttrs ? (uint32_t)mImpl->mMappedAttrs->Count() : 0;
+  MOZ_ASSERT(mImpl && mImpl->mMappedAttrs);
+  return static_cast<uint32_t>(mImpl->mMappedAttrs->Count());
 }
 
 nsresult
@@ -548,29 +476,19 @@ AttrArray::EnsureCapacityToClone(const AttrArray& aOther)
   MOZ_ASSERT(!mImpl, "AttrArray::EnsureCapacityToClone requires the array be empty when called");
 
   uint32_t attrCount = aOther.NonMappedAttrCount();
-
-  if (attrCount == 0) {
+  if (!attrCount) {
     return NS_OK;
   }
 
   // No need to use a CheckedUint32 because we are cloning. We know that we
   // have already allocated an AttrArray of this size.
-  uint32_t size = attrCount;
-  size *= ATTRSIZE;
-  uint32_t totalSize = size;
-  totalSize += NS_IMPL_EXTRA_SIZE;
-
-  mImpl = static_cast<Impl*>(malloc(totalSize * sizeof(void*)));
+  mImpl.reset(static_cast<Impl*>(malloc(
+    Impl::AllocationSizeForAttributes(attrCount))));
   NS_ENSURE_TRUE(mImpl, NS_ERROR_OUT_OF_MEMORY);
 
   mImpl->mMappedAttrs = nullptr;
-  mImpl->mBufferSize = size;
-
-  // The array is now the right size, but we should reserve the correct
-  // number of slots for attributes so that children don't get written into
-  // that part of the array (which will then need to be moved later).
-  memset(static_cast<void*>(mImpl->mBuffer), 0, sizeof(InternalAttr) * attrCount);
-  SetAttrSlotCount(attrCount);
+  mImpl->mCapacity = attrCount;
+  mImpl->mAttrCount = 0;
 
   return NS_OK;
 }
@@ -578,84 +496,58 @@ AttrArray::EnsureCapacityToClone(const AttrArray& aOther)
 bool
 AttrArray::GrowBy(uint32_t aGrowSize)
 {
-  CheckedUint32 size = 0;
-  if (mImpl) {
-    size += mImpl->mBufferSize;
-    size += NS_IMPL_EXTRA_SIZE;
-    if (!size.isValid()) {
-      return false;
-    }
-  }
+  const uint32_t kLinearThreshold = 16;
+  const uint32_t kLinearGrowSize = 4;
 
-  CheckedUint32 minSize = size.value();
-  minSize += aGrowSize;
-  if (!minSize.isValid()) {
+  CheckedUint32 capacity = mImpl ? mImpl->mCapacity : 0;
+  CheckedUint32 minCapacity = capacity;
+  minCapacity += aGrowSize;
+  if (!minCapacity.isValid()) {
     return false;
   }
 
-  if (minSize.value() <= ATTRCHILD_ARRAY_LINEAR_THRESHOLD) {
+  if (capacity.value() <= kLinearThreshold) {
     do {
-      size += ATTRCHILD_ARRAY_GROWSIZE;
-      if (!size.isValid()) {
+      capacity += kLinearGrowSize;
+      if (!capacity.isValid()) {
         return false;
       }
-    } while (size.value() < minSize.value());
-  }
-  else {
-    uint32_t shift = mozilla::CeilingLog2(minSize.value());
+    } while (capacity.value() < minCapacity.value());
+  } else {
+    uint32_t shift = mozilla::CeilingLog2(minCapacity.value());
     if (shift >= 32) {
       return false;
     }
-
-    size = 1u << shift;
+    capacity = 1u << shift;
   }
 
-  bool needToInitialize = !mImpl;
-  CheckedUint32 neededSize = size;
-  neededSize *= sizeof(void*);
-  if (!neededSize.isValid()) {
+  CheckedUint32 sizeInBytes = capacity.value();
+  sizeInBytes *= sizeof(InternalAttr);
+  if (!sizeInBytes.isValid()) {
     return false;
   }
 
-  Impl* newImpl = static_cast<Impl*>(realloc(mImpl, neededSize.value()));
+  sizeInBytes += sizeof(Impl);
+  if (!sizeInBytes.isValid()) {
+    return false;
+  }
+
+  MOZ_ASSERT(sizeInBytes.value() ==
+               Impl::AllocationSizeForAttributes(capacity.value()));
+
+  const bool needToInitialize = !mImpl;
+  Impl* newImpl = static_cast<Impl*>(realloc(mImpl.release(), sizeInBytes.value()));
   NS_ENSURE_TRUE(newImpl, false);
 
-  mImpl = newImpl;
+  mImpl.reset(newImpl);
 
   // Set initial counts if we didn't have a buffer before
   if (needToInitialize) {
     mImpl->mMappedAttrs = nullptr;
-    SetAttrSlotCount(0);
+    mImpl->mAttrCount = 0;
   }
 
-  mImpl->mBufferSize = size.value() - NS_IMPL_EXTRA_SIZE;
-
-  return true;
-}
-
-bool
-AttrArray::AddAttrSlot()
-{
-  uint32_t slotCount = AttrSlotCount();
-
-  CheckedUint32 size = slotCount;
-  size += 1;
-  size *= ATTRSIZE;
-  if (!size.isValid()) {
-    return false;
-  }
-
-  // Grow buffer if needed
-  if (!(mImpl && mImpl->mBufferSize >= size.value()) &&
-      !GrowBy(ATTRSIZE)) {
-    return false;
-  }
-
-  void** offset = mImpl->mBuffer + slotCount * ATTRSIZE;
-
-  SetAttrSlotCount(slotCount + 1);
-  memset(static_cast<void*>(offset), 0, sizeof(InternalAttr));
-
+  mImpl->mCapacity = capacity.value();
   return true;
 }
 
@@ -666,12 +558,10 @@ AttrArray::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
   if (mImpl) {
     // Don't add the size taken by *mMappedAttrs because it's shared.
 
-    n += aMallocSizeOf(mImpl);
+    n += aMallocSizeOf(mImpl.get());
 
-    uint32_t slotCount = AttrSlotCount();
-    for (uint32_t i = 0; i < slotCount && AttrSlotIsTaken(i); ++i) {
-      nsAttrValue* value = &ATTRS(mImpl)[i].mValue;
-      n += value->SizeOfExcludingThis(aMallocSizeOf);
+    for (const InternalAttr& attr : NonMappedAttrs()) {
+      n += attr.mValue.SizeOfExcludingThis(aMallocSizeOf);
     }
   }
 

dom/base/AttrArray.h

@@ -13,6 +13,8 @@
 
 #include "mozilla/Attributes.h"
 #include "mozilla/MemoryReporting.h"
+#include "mozilla/UniquePtr.h"
+#include "mozilla/Span.h"
 #include "mozilla/dom/BorrowedAttrInfo.h"
 
 #include "nscore.h"
@@ -27,24 +29,23 @@ class nsHTMLStyleSheet;
 class nsRuleWalker;
 class nsMappedAttributeElement;
 
-#define ATTRCHILD_ARRAY_GROWSIZE 8
-#define ATTRCHILD_ARRAY_LINEAR_THRESHOLD 32
-
-#define ATTRSIZE (sizeof(InternalAttr) / sizeof(void*))
-
 class AttrArray
 {
   typedef mozilla::dom::BorrowedAttrInfo BorrowedAttrInfo;
 public:
-  AttrArray();
-  ~AttrArray();
+  AttrArray() = default;
+  ~AttrArray() = default;
 
   bool HasAttrs() const
   {
-    return MappedAttrCount() || (AttrSlotCount() && AttrSlotIsTaken(0));
+    return NonMappedAttrCount() || MappedAttrCount();
+  }
+
+  uint32_t AttrCount() const
+  {
+    return NonMappedAttrCount() + MappedAttrCount();
   }
 
-  uint32_t AttrCount() const;
   const nsAttrValue* GetAttr(nsAtom* aLocalName,
                              int32_t aNamespaceID = kNameSpaceID_None) const;
   // As above but using a string attr name and always using
@@ -130,10 +131,17 @@ private:
   AttrArray(const AttrArray& aOther) = delete;
   AttrArray& operator=(const AttrArray& aOther) = delete;
 
-  void Clear();
+  uint32_t NonMappedAttrCount() const
+  {
+    return mImpl ? mImpl->mAttrCount : 0;
+  }
 
-  uint32_t NonMappedAttrCount() const;
-  uint32_t MappedAttrCount() const;
+  uint32_t MappedAttrCount() const
+  {
+    return mImpl && mImpl->mMappedAttrs ? DoGetMappedAttrCount() : 0;
+  }
+
+  uint32_t DoGetMappedAttrCount() const;
 
   // Returns a non-null zero-refcount object.
   nsMappedAttributes*
@@ -143,29 +151,19 @@ private:
                       int32_t aAttrCount = 1);
   nsresult MakeMappedUnique(nsMappedAttributes* aAttributes);
 
-  uint32_t AttrSlotsSize() const
-  {
-    return AttrSlotCount() * ATTRSIZE;
-  }
-
-  uint32_t AttrSlotCount() const
-  {
-    return mImpl ? mImpl->mAttrCount : 0;
-  }
-
-  bool AttrSlotIsTaken(uint32_t aSlot) const
-  {
-    MOZ_ASSERT(aSlot < AttrSlotCount(), "out-of-bounds");
-    return mImpl->mBuffer[aSlot * ATTRSIZE];
-  }
-
-  void SetAttrSlotCount(uint32_t aCount)
-  {
-    mImpl->mAttrCount = aCount;
-  }
-
   bool GrowBy(uint32_t aGrowSize);
-  bool AddAttrSlot();
+
+  struct InternalAttr;
+
+  // Tries to create an attribute, growing the buffer if needed, with the given
+  // name and value.
+  //
+  // The value is moved from the argument.
+  //
+  // `Name` can be anything you construct a `nsAttrName` with (either an atom or
+  // a NodeInfo pointer).
+  template<typename Name>
+  nsresult AddNewAttribute(Name*, nsAttrValue&);
 
   /**
    * Guts of SetMappedAttrStyleSheet for the rare case when we have mapped attrs
@@ -183,14 +181,50 @@ private:
     nsAttrValue mValue;
   };
 
-  struct Impl {
+  class Impl
+  {
+  public:
+
+    constexpr static size_t AllocationSizeForAttributes(uint32_t aAttrCount)
+    {
+      return sizeof(Impl) + aAttrCount * sizeof(InternalAttr);
+    }
+
+    mozilla::Span<const InternalAttr> NonMappedAttrs() const
+    {
+      return mozilla::MakeSpan(static_cast<const InternalAttr*>(mBuffer), mAttrCount);
+    }
+
+    mozilla::Span<InternalAttr> NonMappedAttrs()
+    {
+      return mozilla::MakeSpan(static_cast<InternalAttr*>(mBuffer), mAttrCount);
+    }
+
+    Impl(const Impl&) = delete;
+    Impl(Impl&&) = delete;
+    ~Impl();
+
     uint32_t mAttrCount;
-    uint32_t mBufferSize;
+    uint32_t mCapacity; // In number of InternalAttrs
+
+    // Manually refcounted.
     nsMappedAttributes* mMappedAttrs;
-    void* mBuffer[1];
+
+    // Allocated in the same buffer as `Impl`.
+    InternalAttr mBuffer[0];
   };
 
-  Impl* mImpl;
+  mozilla::Span<InternalAttr> NonMappedAttrs()
+  {
+    return mImpl ? mImpl->NonMappedAttrs() : mozilla::Span<InternalAttr>();
+  }
+
+  mozilla::Span<const InternalAttr> NonMappedAttrs() const
+  {
+    return mImpl ? mImpl->NonMappedAttrs() : mozilla::Span<const InternalAttr>();
+  }
+
+  mozilla::UniquePtr<Impl> mImpl;
 };
 
 #endif