Bug 1308317 - Part 8: Just use an nsTAutoArray. r=froydnj
☠☠ backed out by 9ab31bcd2d40 ☠ ☠
authorEric Rahm <erahm@mozilla.com>
Thu, 13 Oct 2016 22:04:42 -0700
changeset 317944 96d1b7238832ebf3e7e03053db19a8e557f6ca42
parent 317943 928ad5e9337239c5c8effc00ac42fb1140351a71
child 317945 1391a2889aeb2bdd61ad6ef838e65826e35aabc2
child 317999 f824c01ff5ca54dfaf16d8a64110ef8ca2ddbac2
push id33170
push usercbook@mozilla.com
push dateFri, 14 Oct 2016 10:37:07 +0000
treeherderautoland@0d101ebfd95c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1308317
milestone52.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 1308317 - Part 8: Just use an nsTAutoArray. r=froydnj This switches over from using a half-baked auto array to nsTAutoArray. MozReview-Commit-ID: 6FR2SjOhoZR
xpcom/ds/nsSupportsArray.cpp
xpcom/ds/nsSupportsArray.h
--- a/xpcom/ds/nsSupportsArray.cpp
+++ b/xpcom/ds/nsSupportsArray.cpp
@@ -1,22 +1,21 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include <stdint.h>
 #include <string.h>
-#include "mozilla/CheckedInt.h"
-#include "mozilla/MathAlgorithms.h"
+
+#include "nsIObjectInputStream.h"
+#include "nsIObjectOutputStream.h"
 #include "nsSupportsArray.h"
 #include "nsSupportsArrayEnumerator.h"
-#include "nsIObjectInputStream.h"
-#include "nsIObjectOutputStream.h"
 
 nsresult
 nsQueryElementAt::operator()(const nsIID& aIID, void** aResult) const
 {
   nsresult status =
     mCollection ? mCollection->QueryElementAt(mIndex, aIID, aResult) :
                   NS_ERROR_NULL_POINTER;
 
@@ -24,79 +23,21 @@ nsQueryElementAt::operator()(const nsIID
     *mErrorPtr = status;
   }
 
   return status;
 }
 
 nsSupportsArray::nsSupportsArray()
 {
-  mArray = mAutoArray;
-  mArraySize = kAutoArraySize;
-  mCount = 0;
 }
 
 nsSupportsArray::~nsSupportsArray()
 {
-  DeleteArray();
-}
-
-bool
-nsSupportsArray::GrowArrayBy(uint32_t aGrowBy)
-{
-  const uint32_t kGrowArrayBy = 8;
-  const uint32_t kLinearThreshold = 16 * sizeof(nsISupports*);
-
-  // We have to grow the array. Grow by kGrowArrayBy slots if we're smaller
-  // than kLinearThreshold bytes, or a power of two if we're larger.
-  // This is much more efficient with most memory allocators, especially
-  // if it's very large, or of the allocator is binned.
-  if (aGrowBy < kGrowArrayBy) {
-    aGrowBy = kGrowArrayBy;
-  }
-
-  CheckedUint32 newCount(mArraySize);
-  newCount += aGrowBy;  // Minimum increase
-  CheckedUint32 newSize(sizeof(mArray[0]));
-  newSize *= newCount;
-
-  if (!newSize.isValid()) {
-    return false;
-  }
-
-  if (newSize.value() >= kLinearThreshold) {
-    // newCount includes enough space for at least kGrowArrayBy new slots.
-    // Select the next power-of-two size in bytes above that if newSize is
-    // not a power of two.
-    if (newSize.value() & (newSize.value() - 1)) {
-      newSize = UINT64_C(1) << mozilla::CeilingLog2(newSize.value());
-      if (!newSize.isValid()) {
-        return false;
-      }
-    }
-
-    newCount = newSize / sizeof(mArray[0]);
-  }
-  // XXX This would be far more efficient in many allocators if we used
-  // XXX PR_Realloc(), etc
-  nsISupports** oldArray = mArray;
-
-  mArray = new nsISupports*[newCount.value()];
-  mArraySize = newCount.value();
-
-  if (oldArray) {                   // need to move old data
-    if (0 < mCount) {
-      ::memcpy(mArray, oldArray, mCount * sizeof(nsISupports*));
-    }
-    if (oldArray != &(mAutoArray[0])) {
-      delete[] oldArray;
-    }
-  }
-
-  return true;
+  Clear();
 }
 
 nsresult
 nsSupportsArray::Create(nsISupports* aOuter, REFNSIID aIID, void** aResult)
 {
   if (aOuter) {
     return NS_ERROR_NO_AGGREGATION;
   }
@@ -107,198 +48,138 @@ nsSupportsArray::Create(nsISupports* aOu
 }
 
 NS_IMPL_ISUPPORTS(nsSupportsArray, nsISupportsArray, nsICollection,
                   nsISerializable)
 
 NS_IMETHODIMP
 nsSupportsArray::Read(nsIObjectInputStream* aStream)
 {
+  // TODO(ER): This used to leak when resizing the array. Not sure if that was
+  // intentional, I'm guessing not.
   nsresult rv;
 
   uint32_t newArraySize;
   rv = aStream->Read32(&newArraySize);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  if (newArraySize <= kAutoArraySize) {
-    if (mArray != mAutoArray) {
-      delete[] mArray;
-      mArray = mAutoArray;
-    }
-    newArraySize = kAutoArraySize;
-  } else {
-    if (newArraySize <= mArraySize) {
-      // Keep non-default-size mArray, it's more than big enough.
-      newArraySize = mArraySize;
-    } else {
-      nsISupports** array = new nsISupports*[newArraySize];
-      if (mArray != mAutoArray) {
-        delete[] mArray;
-      }
-      mArray = array;
-    }
-  }
-  mArraySize = newArraySize;
-
-  rv = aStream->Read32(&mCount);
+  uint32_t count;
+  rv = aStream->Read32(&count);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  NS_ASSERTION(mCount <= mArraySize, "overlarge mCount!");
-  if (mCount > mArraySize) {
-    mCount = mArraySize;
+  NS_ASSERTION(count <= newArraySize, "overlarge mCount!");
+  if (count > newArraySize) {
+    count = newArraySize;
   }
 
-  for (uint32_t i = 0; i < mCount; i++) {
-    rv = aStream->ReadObject(true, &mArray[i]);
+  // Don't clear out our array until we know we have enough space for the new
+  // one and have successfully copied everything out of the stream.
+  ISupportsArray tmp;
+  if (!tmp.SetCapacity(newArraySize, mozilla::fallible)) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+
+  auto elems = tmp.AppendElements(count, mozilla::fallible);
+  for (uint32_t i = 0; i < count; i++) {
+    rv = aStream->ReadObject(true, &elems[i]);
     if (NS_FAILED(rv)) {
       return rv;
     }
   }
 
+  // Now clear out existing refs and replace with the new array.
+  for (auto& item : mArray) {
+    NS_IF_RELEASE(item);
+  }
+
+  mArray.Clear();
+  mArray.SwapElements(tmp);
+
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsSupportsArray::Write(nsIObjectOutputStream* aStream)
 {
   nsresult rv;
 
-  rv = aStream->Write32(mArraySize);
+  rv = aStream->Write32(mArray.Capacity());
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  rv = aStream->Write32(mCount);
+  rv = aStream->Write32(mArray.Length());
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  for (uint32_t i = 0; i < mCount; i++) {
-    rv = aStream->WriteObject(mArray[i], true);
+  for (auto& item : mArray) {
+    rv = aStream->WriteObject(item, true);
     if (NS_FAILED(rv)) {
       return rv;
     }
   }
 
   return NS_OK;
 }
 
-void
-nsSupportsArray::DeleteArray(void)
-{
-  Clear();
-  if (mArray != &(mAutoArray[0])) {
-    delete[] mArray;
-    mArray = mAutoArray;
-    mArraySize = kAutoArraySize;
-  }
-}
-
 NS_IMETHODIMP
 nsSupportsArray::GetElementAt(uint32_t aIndex, nsISupports** aOutPtr)
 {
-  *aOutPtr = nullptr;
-  if (aIndex < mCount) {
-    NS_IF_ADDREF(*aOutPtr = mArray[aIndex]);
-  }
+  NS_IF_ADDREF(*aOutPtr = mArray.SafeElementAt(aIndex, nullptr));
   return NS_OK;
 }
 
 NS_IMETHODIMP_(int32_t)
 nsSupportsArray::IndexOf(const nsISupports* aPossibleElement)
 {
-  const nsISupports** start = (const nsISupports**)mArray;  // work around goofy compiler behavior
-  const nsISupports** ep = start;
-  const nsISupports** end = (start + mCount);
-  while (ep < end) {
-    if (aPossibleElement == *ep) {
-      return (ep - start);
-    }
-    ep++;
-  }
-  return -1;
+  return mArray.IndexOf(aPossibleElement);
 }
 
 NS_IMETHODIMP_(int32_t)
 nsSupportsArray::LastIndexOf(const nsISupports* aPossibleElement)
 {
-  if (0 < mCount) {
-    const nsISupports** start = (const nsISupports**)mArray;  // work around goofy compiler behavior
-    const nsISupports** ep = (start + mCount);
-    while (start <= --ep) {
-      if (aPossibleElement == *ep) {
-        return (ep - start);
-      }
-    }
-  }
-  return -1;
+  return mArray.LastIndexOf(aPossibleElement);
 }
 
 NS_IMETHODIMP_(bool)
 nsSupportsArray::InsertElementAt(nsISupports* aElement, uint32_t aIndex)
 {
-  if (aIndex <= mCount) {
-    CheckedUint32 newCount(mCount);
-    newCount += 1;
-    if (!newCount.isValid()) {
-      return false;
-    }
-
-    if (mArraySize < newCount.value()) {
-      // need to grow the array
-      if (!GrowArrayBy(1)) {
-        return false;
-      }
-    }
 
-    // Could be slightly more efficient if GrowArrayBy knew about the
-    // split, but the difference is trivial.
-    uint32_t slide = (mCount - aIndex);
-    if (0 < slide) {
-      ::memmove(mArray + aIndex + 1, mArray + aIndex,
-                slide * sizeof(nsISupports*));
-    }
+  if (aIndex > mArray.Length() ||
+      !mArray.InsertElementAt(aIndex, aElement, mozilla::fallible)) {
+    return false;
+  }
 
-    mArray[aIndex] = aElement;
-    NS_IF_ADDREF(aElement);
-    mCount++;
-
-    return true;
-  }
-  return false;
+  NS_IF_ADDREF(aElement);
+  return true;
 }
 
 NS_IMETHODIMP_(bool)
 nsSupportsArray::ReplaceElementAt(nsISupports* aElement, uint32_t aIndex)
 {
-  if (aIndex < mCount) {
+  if (aIndex < mArray.Length()) {
     NS_IF_ADDREF(aElement);  // addref first in case it's the same object!
     NS_IF_RELEASE(mArray[aIndex]);
     mArray[aIndex] = aElement;
     return true;
   }
   return false;
 }
 
 NS_IMETHODIMP_(bool)
 nsSupportsArray::RemoveElementAt(uint32_t aIndex)
 {
-  if (aIndex + 1 <= mCount) {
+  if (aIndex + 1 <= mArray.Length()) {
     NS_IF_RELEASE(mArray[aIndex]);
-
-    mCount -= 1;
-    int32_t slide = (mCount - aIndex);
-    if (0 < slide) {
-      ::memmove(mArray + aIndex, mArray + aIndex + 1,
-                slide * sizeof(nsISupports*));
-    }
+    mArray.RemoveElementAt(aIndex);
     return true;
   }
   return false;
 }
 
 NS_IMETHODIMP
 nsSupportsArray::RemoveElement(nsISupports* aElement)
 {
@@ -308,45 +189,29 @@ nsSupportsArray::RemoveElement(nsISuppor
   }
 
   return NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
 nsSupportsArray::Clear(void)
 {
-  if (0 < mCount) {
-    do {
-      --mCount;
-      NS_IF_RELEASE(mArray[mCount]);
-    } while (0 != mCount);
+  for (auto& item : mArray) {
+    NS_IF_RELEASE(item);
   }
+
+  mArray.Clear();
+
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsSupportsArray::Compact(void)
 {
-  if ((mArraySize != mCount) && (kAutoArraySize < mArraySize)) {
-    nsISupports** oldArray = mArray;
-    if (mCount <= kAutoArraySize) {
-      mArray = mAutoArray;
-      mArraySize = kAutoArraySize;
-    } else {
-      mArray = new nsISupports*[mCount];
-      if (!mArray) {
-        mArray = oldArray;
-        return NS_OK;
-      }
-      mArraySize = mCount;
-    }
-
-    ::memcpy(mArray, oldArray, mCount * sizeof(nsISupports*));
-    delete[] oldArray;
-  }
+  mArray.Compact();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsSupportsArray::Enumerate(nsIEnumerator** aResult)
 {
   nsSupportsArrayEnumerator* e = new nsSupportsArrayEnumerator(this);
   *aResult = e;
@@ -358,20 +223,20 @@ NS_IMETHODIMP
 nsSupportsArray::Clone(nsISupportsArray** aResult)
 {
   nsCOMPtr<nsISupportsArray> newArray;
   nsresult rv = NS_NewISupportsArray(getter_AddRefs(newArray));
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
 
-  uint32_t count = 0;
-  Count(&count);
-  for (uint32_t i = 0; i < count; i++) {
-    if (!newArray->InsertElementAt(mArray[i], i)) {
+  for (auto& item : mArray) {
+    // AppendElement does an odd cast of bool to nsresult, we just cast back
+    // here.
+    if (!(bool)newArray->AppendElement(item)) {
       return NS_ERROR_OUT_OF_MEMORY;
     }
   }
 
   newArray.forget(aResult);
   return NS_OK;
 }
 
--- a/xpcom/ds/nsSupportsArray.h
+++ b/xpcom/ds/nsSupportsArray.h
@@ -3,20 +3,19 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef nsSupportsArray_h__
 #define nsSupportsArray_h__
 
 #include "nsISupportsArray.h"
+#include "nsTArray.h"
 #include "mozilla/Attributes.h"
 
-static const uint32_t kAutoArraySize = 8;
-
 class nsSupportsArray final : public nsISupportsArray
 {
   ~nsSupportsArray(void); // nonvirtual since we're not subclassed
 
 public:
   nsSupportsArray(void);
 
   static MOZ_MUST_USE nsresult
@@ -24,40 +23,38 @@ public:
 
   NS_DECL_THREADSAFE_ISUPPORTS
 
   NS_DECL_NSISERIALIZABLE
 
   // nsICollection methods:
   NS_IMETHOD Count(uint32_t* aResult) override
   {
-    *aResult = mCount;
+    *aResult = mArray.Length();
     return NS_OK;
   }
   NS_IMETHOD GetElementAt(uint32_t aIndex, nsISupports** aResult) override;
   MOZ_MUST_USE NS_IMETHOD
   QueryElementAt(uint32_t aIndex, const nsIID& aIID, void** aResult) override
   {
-    if (aIndex < mCount) {
-      nsISupports* element = mArray[aIndex];
-      if (element) {
-        return element->QueryInterface(aIID, aResult);
-      }
+    nsISupports* element = mArray.SafeElementAt(aIndex, nullptr);
+    if (element) {
+      return element->QueryInterface(aIID, aResult);
     }
     return NS_ERROR_FAILURE;
   }
   MOZ_MUST_USE NS_IMETHOD
   SetElementAt(uint32_t aIndex, nsISupports* aValue) override
   {
     return ReplaceElementAt(aValue, aIndex) ? NS_OK : NS_ERROR_FAILURE;
   }
   MOZ_MUST_USE NS_IMETHOD AppendElement(nsISupports* aElement) override
   {
     // XXX Invalid cast of bool to nsresult (bug 778110)
-    return (nsresult)InsertElementAt(aElement, mCount)/* ? NS_OK : NS_ERROR_FAILURE*/;
+    return (nsresult)InsertElementAt(aElement, mArray.Length())/* ? NS_OK : NS_ERROR_FAILURE*/;
   }
   // XXX this is badly named - should be RemoveFirstElement
   MOZ_MUST_USE NS_IMETHOD RemoveElement(nsISupports* aElement) override;
   NS_IMETHOD Enumerate(nsIEnumerator** aResult) override;
   NS_IMETHOD Clear(void) override;
 
   // nsISupportsArray methods:
   NS_IMETHOD_(int32_t) IndexOf(const nsISupports* aPossibleElement) override;
@@ -88,24 +85,17 @@ public:
   {
     return (RemoveElementAt(aIndex) ? NS_OK : NS_ERROR_FAILURE);
   }
 
   NS_IMETHOD Compact(void) override;
 
   MOZ_MUST_USE NS_IMETHOD Clone(nsISupportsArray** aResult) override;
 
-protected:
-  void DeleteArray(void);
-
-  bool GrowArrayBy(uint32_t aGrowBy);
-
-  nsISupports** mArray;
-  uint32_t mArraySize;
-  uint32_t mCount;
-  nsISupports*  mAutoArray[kAutoArraySize];
-
 private:
   // Copy constructors are not allowed
   explicit nsSupportsArray(const nsISupportsArray& aOther);
+
+  typedef AutoTArray<nsISupports*, 8> ISupportsArray;
+  ISupportsArray mArray;
 };
 
 #endif // nsSupportsArray_h__