Issue #2148 - Make Vector not use AlignedStorage for its inline element storage

See Bug 1338374 1/2

1 files changed, 64 insertions, 35 deletions

diff --git a/mfbt/Vector.h b/mfbt/Vector.h
index cda406b30a..b8fc4617bc 100644
--- a/mfbt/Vector.h
+++ b/mfbt/Vector.h
@@ -277,7 +277,7 @@ struct VectorTesting;
 template<typename T,
          size_t MinInlineCapacity = 0,
          class AllocPolicy = MallocAllocPolicy>
-class Vector final : private AllocPolicy
+class MOZ_NON_PARAM Vector final : private AllocPolicy
 {
   /* utilities */
 
@@ -293,36 +293,39 @@ class Vector final : private AllocPolicy
 
   /* magic constants */
 
-  static const int kMaxInlineBytes = 1024;
-
-  /* compute constants */
+/**
+   * The maximum space allocated for inline element storage.
+   *
+   * We reduce space by what the AllocPolicy base class and prior Vector member
+   * fields likely consume to attempt to play well with binary size classes.
+   */
+  static constexpr size_t kMaxInlineBytes =
+      1024 -
+      (sizeof(AllocPolicy) + sizeof(T*) + sizeof(size_t) + sizeof(size_t));
 
-  /*
-   * Consider element size to be 1 for buffer sizing if there are 0 inline
-   * elements.  This allows us to compile when the definition of the element
-   * type is not visible here.
+  /**
+   * The number of T elements of inline capacity built into this Vector.  This
+   * is usually |MinInlineCapacity|, but it may be less (or zero!) for large T.
    *
-   * Explicit specialization is only allowed at namespace scope, so in order
-   * to keep everything here, we use a dummy template parameter with partial
-   * specialization.
+   * We use a partially-specialized template (not explicit specialization, which
+   * is only allowed at namespace scope) to compute this value.  The benefit is
+   * that |sizeof(T)| need not be computed, and |T| doesn't have to be fully
+   * defined at the time |Vector<T>| appears, if no inline storage is requested.
    */
-  template<int M, int Dummy>
-  struct ElemSize
-  {
-    static const size_t value = sizeof(T);
-  };
-  template<int Dummy>
-  struct ElemSize<0, Dummy>
-  {
-    static const size_t value = 1;
+  template <size_t MinimumInlineCapacity, size_t Dummy>
+  struct ComputeCapacity {
+    static constexpr size_t value =
+        tl::Min<MinimumInlineCapacity, kMaxInlineBytes / sizeof(T)>::value;
   };
 
-  static const size_t kInlineCapacity =
-    tl::Min<MinInlineCapacity, kMaxInlineBytes / ElemSize<MinInlineCapacity, 0>::value>::value;
+  template <size_t Dummy>
+  struct ComputeCapacity<0, Dummy> {
+    static constexpr size_t value = 0;
+  };
 
-  /* Calculate inline buffer size; avoid 0-sized array. */
-  static const size_t kInlineBytes =
-    tl::Max<1, kInlineCapacity * ElemSize<MinInlineCapacity, 0>::value>::value;
+  /** The actual inline capacity in number of elements T.  This may be zero! */
+  static constexpr size_t kInlineCapacity =
+      ComputeCapacity<MinInlineCapacity, 0>::value;
 
   /* member data */
 
@@ -346,8 +349,34 @@ class Vector final : private AllocPolicy
   size_t mReserved;
 #endif
 
-  /* Memory used for inline storage. */
-  AlignedStorage<kInlineBytes> mStorage;
+  /*
+   * Memory used for inline storage.  We want basically this:
+   *
+   *   alignas(T) unsigned char storage[kInlineCapacity * sizeof(T)];
+   *
+   * but C++ forbids zero-sized arrays that might result if we did this.  We fix
+   * this by (again) using partial specialization, defining an array only if
+   * contains at least one element.
+   */
+  template<size_t Capacity, size_t Dummy>
+  struct InlineStorage
+  {
+    alignas(T) unsigned char mBytes[Capacity * sizeof(T)];
+
+    // GCC fails due to -Werror=strict-aliasing if |mBytes| is directly cast to
+    // T*.  Indirecting through this function addresses the problem.
+    void* data() { return mBytes; }
+
+    T* addr() { return static_cast<T*>(data()); }
+  };
+
+  template<size_t Dummy>
+  struct InlineStorage<0, Dummy>
+  {
+    T* addr() { return nullptr; }
+  };
+
+  InlineStorage<kInlineCapacity, 0> mStorage;
 
 #ifdef DEBUG
   friend class ReentrancyGuard;
@@ -363,7 +392,7 @@ class Vector final : private AllocPolicy
 
   T* inlineStorage()
   {
-    return static_cast<T*>(mStorage.addr());
+    return mStorage.addr();
   }
 
   T* beginNoCheck() const
@@ -771,7 +800,7 @@ Vector<T, N, AP>::Vector(AP aAP)
   , mEntered(false)
 #endif
 {
-  mBegin = static_cast<T*>(mStorage.addr());
+  mBegin = inlineStorage();
 }
 
 /* Move constructor. */
@@ -791,7 +820,7 @@ Vector<T, N, AllocPolicy>::Vector(Vector&& aRhs)
 
   if (aRhs.usingInlineStorage()) {
     /* We can't move the buffer over in this case, so copy elements. */
-    mBegin = static_cast<T*>(mStorage.addr());
+    mBegin = inlineStorage();
     Impl::moveConstruct(mBegin, aRhs.beginNoCheck(), aRhs.endNoCheck());
     /*
      * Leave aRhs's mLength, mBegin, mCapacity, and mReserved as they are.
@@ -803,7 +832,7 @@ Vector<T, N, AllocPolicy>::Vector(Vector&& aRhs)
      * in-line storage.
      */
     mBegin = aRhs.mBegin;
-    aRhs.mBegin = static_cast<T*>(aRhs.mStorage.addr());
+    aRhs.mBegin = aRhs.inlineStorage();
     aRhs.mCapacity = kInlineCapacity;
     aRhs.mLength = 0;
 #ifdef DEBUG
@@ -1142,7 +1171,7 @@ Vector<T, N, AP>::clearAndFree()
     return;
   }
   this->free_(beginNoCheck());
-  mBegin = static_cast<T*>(mStorage.addr());
+  mBegin = inlineStorage();
   mCapacity = kInlineCapacity;
 #ifdef DEBUG
   mReserved = 0;
@@ -1370,7 +1399,7 @@ Vector<T, N, AP>::extractRawBuffer()
   }
 
   T* ret = mBegin;
-  mBegin = static_cast<T*>(mStorage.addr());
+  mBegin = inlineStorage();
   mLength = 0;
   mCapacity = kInlineCapacity;
 #ifdef DEBUG
@@ -1396,7 +1425,7 @@ Vector<T, N, AP>::extractOrCopyRawBuffer()
 
   Impl::moveConstruct(copy, beginNoCheck(), endNoCheck());
   Impl::destroy(beginNoCheck(), endNoCheck());
-  mBegin = static_cast<T*>(mStorage.addr());
+  mBegin = inlineStorage();
   mLength = 0;
   mCapacity = kInlineCapacity;
 #ifdef DEBUG
@@ -1424,7 +1453,7 @@ Vector<T, N, AP>::replaceRawBuffer(T* aP, size_t aLength)
      * otherwise be acceptable.  Maybe this behaviour should be
      * specifiable with an argument to this function.
      */
-    mBegin = static_cast<T*>(mStorage.addr());
+    mBegin = inlineStorage();
     mLength = aLength;
     mCapacity = kInlineCapacity;
     Impl::moveConstruct(mBegin, aP, aP + aLength);