Oilpan: Vector::reserveCapacity shouldn't reallocate backing storage always

Source/wtf/Vector.h

 /*
  *  Copyright (C) 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Library General Public
  *  License as published by the Free Software Foundation; either
  *  version 2 of the License, or (at your option) any later version.
  *
  *  This library is distributed in the hope that it will be useful,
  *  but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 341 matching lines @@
         {
             deallocateBuffer(m_buffer);
             m_buffer = 0;
         }
 
         void deallocateBuffer(T* bufferToDeallocate)
         {
             Allocator::backingFree(bufferToDeallocate);
         }
 
+        bool expandBuffer(size_t newCapacity)
+        {
+            size_t sizeToAllocate = allocationSize(newCapacity);
+            if (Allocator::backingExpand(m_buffer, sizeToAllocate)) {
+                m_capacity = sizeToAllocate / sizeof(T);
+                return true;
+            }
+            return false;
+        }
+
         void resetBufferPointer()
         {
             m_buffer = 0;
             m_capacity = 0;
         }
 
         void swapVectorBuffer(VectorBuffer<T, 0, Allocator>& other)
         {
             std::swap(m_buffer, other.m_buffer);
             std::swap(m_capacity, other.m_capacity);
@@ skipping 49 matching lines @@
         {
             Allocator::backingFree(bufferToDeallocate);
         }
 
         void deallocateBuffer(T* bufferToDeallocate)
         {
             if (UNLIKELY(bufferToDeallocate != inlineBuffer()))
                 reallyDeallocateBuffer(bufferToDeallocate);
         }
 
+        bool expandBuffer(size_t newCapacity)
+        {
+            if (newCapacity <= inlineCapacity)

[tkent, 2014/11/14 03:54:25]

Is this check necessary?  The callsite already has a similar check.

[haraken, 2014/11/14 04:32:45]

Removed.

+                return true;
+            if (m_buffer == inlineBuffer())
+                return false;
+
+            size_t sizeToAllocate = allocationSize(newCapacity);
+            if (Allocator::backingExpand(m_buffer, sizeToAllocate)) {
+                m_capacity = sizeToAllocate / sizeof(T);
+                return true;
+            }
+            return false;
+        }
+
         void resetBufferPointer()
         {
             m_buffer = inlineBuffer();
             m_capacity = inlineCapacity;
         }
 
         void allocateBuffer(size_t newCapacity)
         {
             // FIXME: This should ASSERT(!m_buffer) to catch misuse/leaks.
             if (newCapacity > inlineCapacity)
@@ skipping 501 matching lines @@
         m_size = size;
     }
 
     template<typename T, size_t inlineCapacity, typename Allocator>
     void Vector<T, inlineCapacity, Allocator>::reserveCapacity(size_t newCapacity)
     {
         if (UNLIKELY(newCapacity <= capacity()))
             return;
         T* oldBuffer = begin();
         T* oldEnd = end();
+        if (Base::expandBuffer(newCapacity))

[tkent, 2014/11/14 03:54:25]

I'm afraid this change causes performance regression on non-Oilpan.  Did you
confirm blink_perf tests didn't regress on non-Oilpan?

[haraken, 2014/11/14 04:32:45]

I observe no regression in parser benchmarks that append a lot of items to
vectors in Linux. Hopefully this branch is removed by the compiler because
Base::expandBuffer always return false for DefaultAllocator.

I'm not sure of Windows, but let me land it to see how it goes in perf bots.

+            return;
         Base::allocateBuffer(newCapacity);
         TypeOperations::move(oldBuffer, oldEnd, begin());
         Base::deallocateBuffer(oldBuffer);
     }
 
     template<typename T, size_t inlineCapacity, typename Allocator>
     inline void Vector<T, inlineCapacity, Allocator>::reserveInitialCapacity(size_t initialCapacity)
     {
         ASSERT(!m_size);
         ASSERT(capacity() == inlineCapacity);
@@ skipping 229 matching lines @@
     struct NeedsTracing<Vector<T, N> > {
         static const bool value = false;
     };
 #endif
 
 } // namespace WTF
 
 using WTF::Vector;
 
 #endif // WTF_Vector_h