Bug 1105261 - Revert fresh vectors to not prereserving their inline allocation space, because the guaranteed extent of that space is an implementation detail. r=nbp
authorJeff Walden <jwalden@mit.edu>
Wed, 26 Nov 2014 16:01:19 -0500
changeset 218222 68388b632918ef430094b6b42139e4e400fd068b
parent 218221 7873b21367dbc9a7138974cf4f833c768587714e
child 218223 3361018e53e54570b0ca2f06afe49b5655d6f81e
push id10233
push usercbook@mozilla.com
push dateTue, 02 Dec 2014 11:07:19 +0000
treeherderfx-team@e16e9086a245 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnbp
bugs1105261
milestone37.0a1
Bug 1105261 - Revert fresh vectors to not prereserving their inline allocation space, because the guaranteed extent of that space is an implementation detail. r=nbp
js/src/asmjs/AsmJSLink.cpp
js/src/asmjs/AsmJSValidate.cpp
js/src/frontend/TokenStream.cpp
js/src/jsreflect.cpp
mfbt/Vector.h
mfbt/tests/TestVector.cpp
mfbt/tests/moz.build
--- a/js/src/asmjs/AsmJSLink.cpp
+++ b/js/src/asmjs/AsmJSLink.cpp
@@ -822,17 +822,19 @@ HandleDynamicLinkFailure(JSContext *cx, 
 
     RootedFunction fun(cx, NewFunction(cx, NullPtr(), nullptr, 0, JSFunction::INTERPRETED,
                                        cx->global(), name, JSFunction::FinalizeKind,
                                        TenuredObject));
     if (!fun)
         return false;
 
     AutoNameVector formals(cx);
-    formals.reserve(3);
+    if (!formals.reserve(3))
+        return false;
+
     if (module.globalArgumentName())
         formals.infallibleAppend(module.globalArgumentName());
     if (module.importArgumentName())
         formals.infallibleAppend(module.importArgumentName());
     if (module.bufferArgumentName())
         formals.infallibleAppend(module.bufferArgumentName());
 
     CompileOptions options(cx);
--- a/js/src/asmjs/AsmJSValidate.cpp
+++ b/js/src/asmjs/AsmJSValidate.cpp
@@ -8323,17 +8323,18 @@ GenerateFFIInterpExit(ModuleCompiler &m,
     MacroAssembler &masm = m.masm();
     MOZ_ASSERT(masm.framePushed() == 0);
 
     // Argument types for InvokeFromAsmJS_*:
     static const MIRType typeArray[] = { MIRType_Pointer,   // exitDatum
                                          MIRType_Int32,     // argc
                                          MIRType_Pointer }; // argv
     MIRTypeVector invokeArgTypes(m.cx());
-    invokeArgTypes.infallibleAppend(typeArray, ArrayLength(typeArray));
+    if (!invokeArgTypes.append(typeArray, ArrayLength(typeArray)))
+        return false;
 
     // At the point of the call, the stack layout shall be (sp grows to the left):
     //   | stack args | padding | Value argv[] | padding | retaddr | caller stack args |
     // The padding between stack args and argv ensures that argv is aligned. The
     // padding between argv and retaddr ensures that sp is aligned.
     unsigned offsetToArgv = AlignBytes(StackArgBytes(invokeArgTypes), sizeof(double));
     unsigned argvBytes = Max<size_t>(1, exit.sig().args().length()) * sizeof(Value);
     unsigned framePushed = StackDecrementForCall(masm, ABIStackAlignment, offsetToArgv + argvBytes);
@@ -8440,17 +8441,19 @@ GenerateFFIIonExit(ModuleCompiler &m, co
     unsigned ionArgBytes = 3 * sizeof(size_t) + (1 + exit.sig().args().length()) * sizeof(Value);
     unsigned totalIonBytes = offsetToIonArgs + ionArgBytes + MaybeSavedGlobalReg;
     unsigned ionFrameSize = StackDecrementForCall(masm, AsmJSStackAlignment, totalIonBytes);
 
     // Coercion calls use the following stack layout (sp grows to the left):
     //   | stack args | padding | Value argv[1] | ...
     // The padding between args and argv ensures that argv is aligned.
     MIRTypeVector coerceArgTypes(m.cx());
-    coerceArgTypes.infallibleAppend(MIRType_Pointer); // argv
+    if (!coerceArgTypes.append(MIRType_Pointer)) // argv
+        return false;
+
     unsigned offsetToCoerceArgv = AlignBytes(StackArgBytes(coerceArgTypes), sizeof(double));
     unsigned totalCoerceBytes = offsetToCoerceArgv + sizeof(Value) + MaybeSavedGlobalReg;
     unsigned coerceFrameSize = StackDecrementForCall(masm, AsmJSStackAlignment, totalCoerceBytes);
 
     unsigned framePushed = Max(ionFrameSize, coerceFrameSize);
 
     Label begin;
     GenerateAsmJSExitPrologue(masm, framePushed, AsmJSExit::JitFFI, &begin);
@@ -8700,16 +8703,19 @@ GenerateFFIExits(ModuleCompiler &m, cons
 //     (coming out) and preserve non-volatile registers.
 static bool
 GenerateBuiltinThunk(ModuleCompiler &m, AsmJSExit::BuiltinKind builtin)
 {
     MacroAssembler &masm = m.masm();
     MOZ_ASSERT(masm.framePushed() == 0);
 
     MIRTypeVector argTypes(m.cx());
+    if (!argTypes.reserve(2))
+        return false;
+
     switch (builtin) {
       case AsmJSExit::Builtin_ToInt32:
         argTypes.infallibleAppend(MIRType_Int32);
         break;
 #if defined(JS_CODEGEN_ARM)
       case AsmJSExit::Builtin_IDivMod:
       case AsmJSExit::Builtin_UDivMod:
         argTypes.infallibleAppend(MIRType_Int32);
--- a/js/src/frontend/TokenStream.cpp
+++ b/js/src/frontend/TokenStream.cpp
@@ -146,17 +146,17 @@ TokenStream::SourceCoords::SourceCoords(
     // which fixes the GCC/clang error, but causes bustage on Windows.  Sigh.
     //
     uint32_t maxPtr = MAX_PTR;
 
     // The first line begins at buffer offset 0.  MAX_PTR is the sentinel.  The
     // appends cannot fail because |lineStartOffsets_| has statically-allocated
     // elements.
     MOZ_ASSERT(lineStartOffsets_.capacity() >= 2);
-    (void)lineStartOffsets_.reserve(2);
+    MOZ_ALWAYS_TRUE(lineStartOffsets_.reserve(2));
     lineStartOffsets_.infallibleAppend(0);
     lineStartOffsets_.infallibleAppend(maxPtr);
 }
 
 MOZ_ALWAYS_INLINE void
 TokenStream::SourceCoords::add(uint32_t lineNum, uint32_t lineStartOffset)
 {
     uint32_t lineIndex = lineNumToIndex(lineNum);
--- a/js/src/jsreflect.cpp
+++ b/js/src/jsreflect.cpp
@@ -2107,17 +2107,17 @@ ASTSerializer::let(ParseNode *pn, bool e
 bool
 ASTSerializer::importDeclaration(ParseNode *pn, MutableHandleValue dst)
 {
     MOZ_ASSERT(pn->isKind(PNK_IMPORT));
     MOZ_ASSERT(pn->pn_left->isKind(PNK_IMPORT_SPEC_LIST));
     MOZ_ASSERT(pn->pn_right->isKind(PNK_STRING));
 
     NodeVector elts(cx);
-    if (!elts.reserve(pn->pn_count))
+    if (!elts.reserve(pn->pn_left->pn_count))
         return false;
 
     for (ParseNode *next = pn->pn_left->pn_head; next; next = next->pn_next) {
         RootedValue elt(cx);
         if (!importSpecifier(next, &elt))
             return false;
         elts.infallibleAppend(elt);
     }
@@ -2146,17 +2146,17 @@ ASTSerializer::exportDeclaration(ParseNo
     MOZ_ASSERT_IF(pn->isKind(PNK_EXPORT_FROM), pn->pn_right->isKind(PNK_STRING));
 
     RootedValue decl(cx, NullValue());
     NodeVector elts(cx);
 
     ParseNode *kid = pn->isKind(PNK_EXPORT) ? pn->pn_kid : pn->pn_left;
     switch (ParseNodeKind kind = kid->getKind()) {
       case PNK_EXPORT_SPEC_LIST:
-        if (!elts.reserve(pn->pn_count))
+        if (!elts.reserve(pn->pn_left->pn_count))
             return false;
 
         for (ParseNode *next = pn->pn_left->pn_head; next; next = next->pn_next) {
             RootedValue elt(cx);
             if (next->isKind(PNK_EXPORT_SPEC)) {
                 if (!exportSpecifier(next, &elt))
                     return false;
             } else {
--- a/mfbt/Vector.h
+++ b/mfbt/Vector.h
@@ -210,16 +210,20 @@ struct VectorImpl<T, N, AP, ThisVector, 
     }
     aV.mBegin = newbuf;
     /* aV.mLength is unchanged. */
     aV.mCapacity = aNewCap;
     return true;
   }
 };
 
+// A struct for TestVector.cpp to access private internal fields.
+// DO NOT DEFINE IN YOUR OWN CODE.
+struct VectorTesting;
+
 } // namespace detail
 
 /*
  * A CRTP base class for vector-like classes.  Unless you really really want
  * your own vector class -- and you almost certainly don't -- you should use
  * mozilla::Vector instead!
  *
  * See mozilla::Vector for interface requirements.
@@ -228,16 +232,18 @@ template<typename T, size_t N, class All
 class VectorBase : private AllocPolicy
 {
   /* utilities */
 
   static const bool kElemIsPod = IsPod<T>::value;
   typedef detail::VectorImpl<T, N, AllocPolicy, ThisVector, kElemIsPod> Impl;
   friend struct detail::VectorImpl<T, N, AllocPolicy, ThisVector, kElemIsPod>;
 
+  friend struct detail::VectorTesting;
+
   bool growStorageBy(size_t aIncr);
   bool convertToHeapStorage(size_t aNewCap);
 
   /* magic constants */
 
   static const int kMaxInlineBytes = 1024;
 
   /* compute constants */
@@ -322,20 +328,27 @@ class VectorBase : private AllocPolicy
   }
 
   const T* endNoCheck() const
   {
     return mBegin + mLength;
   }
 
 #ifdef DEBUG
+  /**
+   * The amount of explicitly allocated space in this vector that is immediately
+   * available to be filled by appending additional elements.  This value is
+   * always greater than or equal to |length()| -- the vector's actual elements
+   * are implicitly reserved.  This value is always less than or equal to
+   * |capacity()|.  It may be explicitly increased using the |reserve()| method.
+   */
   size_t reserved() const
   {
+    MOZ_ASSERT(mLength <= mReserved);
     MOZ_ASSERT(mReserved <= mCapacity);
-    MOZ_ASSERT(mLength <= mReserved);
     return mReserved;
   }
 #endif
 
   /* Append operations guaranteed to succeed due to pre-reserved space. */
   template<typename U> void internalAppend(U&& aU);
   template<typename U, size_t O, class BP, class UV>
   void internalAppendAll(const VectorBase<U, O, BP, UV>& aU);
@@ -445,18 +458,22 @@ public:
 
   /**
    * Given that the vector is empty and has no inline storage, grow to
    * |capacity|.
    */
   bool initCapacity(size_t aRequest);
 
   /**
-   * If reserve(length() + N) succeeds, the N next appends are guaranteed to
-   * succeed.
+   * If reserve(aRequest) succeeds and |aRequest >= length()|, then appending
+   * |aRequest - length()| elements, in any sequence of append/appendAll calls,
+   * is guaranteed to succeed.
+   *
+   * A request to reserve an amount less than the current length does not affect
+   * reserved space.
    */
   bool reserve(size_t aRequest);
 
   /**
    * Destroy elements in the range [end() - aIncr, end()). Does not deallocate
    * or unreserve storage for those elements.
    */
   void shrinkBy(size_t aIncr);
@@ -617,17 +634,17 @@ private:
 
 template<typename T, size_t N, class AP, class TV>
 MOZ_ALWAYS_INLINE
 VectorBase<T, N, AP, TV>::VectorBase(AP aAP)
   : AP(aAP)
   , mLength(0)
   , mCapacity(kInlineCapacity)
 #ifdef DEBUG
-  , mReserved(kInlineCapacity)
+  , mReserved(0)
   , mEntered(false)
 #endif
 {
   mBegin = static_cast<T*>(mStorage.addr());
 }
 
 /* Move constructor. */
 template<typename T, size_t N, class AllocPolicy, class TV>
@@ -657,17 +674,17 @@ VectorBase<T, N, AllocPolicy, TV>::Vecto
      * Take src's buffer, and turn src into an empty vector using
      * in-line storage.
      */
     mBegin = aRhs.mBegin;
     aRhs.mBegin = static_cast<T*>(aRhs.mStorage.addr());
     aRhs.mCapacity = kInlineCapacity;
     aRhs.mLength = 0;
 #ifdef DEBUG
-    aRhs.mReserved = kInlineCapacity;
+    aRhs.mReserved = 0;
 #endif
   }
 }
 
 /* Move assignment. */
 template<typename T, size_t N, class AP, class TV>
 MOZ_ALWAYS_INLINE TV&
 VectorBase<T, N, AP, TV>::operator=(TV&& aRhs)
@@ -936,17 +953,17 @@ VectorBase<T, N, AP, TV>::clearAndFree()
 
   if (usingInlineStorage()) {
     return;
   }
   this->free_(beginNoCheck());
   mBegin = static_cast<T*>(mStorage.addr());
   mCapacity = kInlineCapacity;
 #ifdef DEBUG
-  mReserved = kInlineCapacity;
+  mReserved = 0;
 #endif
 }
 
 template<typename T, size_t N, class AP, class TV>
 inline bool
 VectorBase<T, N, AP, TV>::canAppendWithoutRealloc(size_t aNeeded) const
 {
   return mLength + aNeeded <= mCapacity;
@@ -1150,17 +1167,17 @@ VectorBase<T, N, AP, TV>::extractRawBuff
     /* mBegin, mCapacity are unchanged. */
     mLength = 0;
   } else {
     ret = mBegin;
     mBegin = static_cast<T*>(mStorage.addr());
     mLength = 0;
     mCapacity = kInlineCapacity;
 #ifdef DEBUG
-    mReserved = kInlineCapacity;
+    mReserved = 0;
 #endif
   }
   return ret;
 }
 
 template<typename T, size_t N, class AP, class TV>
 inline void
 VectorBase<T, N, AP, TV>::replaceRawBuffer(T* aP, size_t aLength)
new file mode 100644
--- /dev/null
+++ b/mfbt/tests/TestVector.cpp
@@ -0,0 +1,82 @@
+/* -*- 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 "mozilla/Move.h"
+#include "mozilla/Vector.h"
+
+using mozilla::detail::VectorTesting;
+using mozilla::Move;
+using mozilla::Vector;
+
+struct mozilla::detail::VectorTesting
+{
+  static void testReserved();
+};
+
+void
+mozilla::detail::VectorTesting::testReserved()
+{
+#ifdef DEBUG
+  Vector<bool> bv;
+  MOZ_RELEASE_ASSERT(bv.reserved() == 0);
+
+  MOZ_RELEASE_ASSERT(bv.append(true));
+  MOZ_RELEASE_ASSERT(bv.reserved() == 1);
+
+  Vector<bool> otherbv;
+  MOZ_RELEASE_ASSERT(otherbv.append(false));
+  MOZ_RELEASE_ASSERT(otherbv.append(true));
+  MOZ_RELEASE_ASSERT(bv.appendAll(otherbv));
+  MOZ_RELEASE_ASSERT(bv.reserved() == 3);
+
+  MOZ_RELEASE_ASSERT(bv.reserve(5));
+  MOZ_RELEASE_ASSERT(bv.reserved() == 5);
+
+  MOZ_RELEASE_ASSERT(bv.reserve(1));
+  MOZ_RELEASE_ASSERT(bv.reserved() == 5);
+
+  Vector<bool> bv2(Move(bv));
+  MOZ_RELEASE_ASSERT(bv.reserved() == 0);
+  MOZ_RELEASE_ASSERT(bv2.reserved() == 5);
+
+  bv2.clearAndFree();
+  MOZ_RELEASE_ASSERT(bv2.reserved() == 0);
+
+  Vector<int, 42> iv;
+  MOZ_RELEASE_ASSERT(iv.reserved() == 0);
+
+  MOZ_RELEASE_ASSERT(iv.append(17));
+  MOZ_RELEASE_ASSERT(iv.reserved() == 1);
+
+  Vector<int, 42> otheriv;
+  MOZ_RELEASE_ASSERT(otheriv.append(42));
+  MOZ_RELEASE_ASSERT(otheriv.append(37));
+  MOZ_RELEASE_ASSERT(iv.appendAll(otheriv));
+  MOZ_RELEASE_ASSERT(iv.reserved() == 3);
+
+  MOZ_RELEASE_ASSERT(iv.reserve(5));
+  MOZ_RELEASE_ASSERT(iv.reserved() == 5);
+
+  MOZ_RELEASE_ASSERT(iv.reserve(1));
+  MOZ_RELEASE_ASSERT(iv.reserved() == 5);
+
+  MOZ_RELEASE_ASSERT(iv.reserve(55));
+  MOZ_RELEASE_ASSERT(iv.reserved() == 55);
+
+  Vector<int, 42> iv2(Move(iv));
+  MOZ_RELEASE_ASSERT(iv.reserved() == 0);
+  MOZ_RELEASE_ASSERT(iv2.reserved() == 55);
+
+  iv2.clearAndFree();
+  MOZ_RELEASE_ASSERT(iv2.reserved() == 0);
+#endif
+}
+
+int
+main()
+{
+  VectorTesting::testReserved();
+}
--- a/mfbt/tests/moz.build
+++ b/mfbt/tests/moz.build
@@ -25,16 +25,17 @@ CppUnitTests([
     'TestPair',
     'TestRefPtr',
     'TestRollingMean',
     'TestSHA1',
     'TestSplayTree',
     'TestTypedEnum',
     'TestTypeTraits',
     'TestUniquePtr',
+    'TestVector',
     'TestWeakPtr',
 ])
 
 if not CONFIG['MOZ_ASAN']:
     CppUnitTests([
         'TestPoisonArea',
     ])