Auto-shrink the buffer in base::circular_deque.

Reallocates the underlying buffer when the wasted elements are equal to or greated than the number of used elements.

This will prevent wasting too much memory if many items are added to a deque.

Change-Id: I395a45528e78c3b427be2ceb542d7140f06367af
Reviewed-on: https://chromium-review.googlesource.com/613678
Commit-Queue: Brett Wilson <brettw@chromium.org>
Reviewed-by: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: Dmitry Skiba <dskiba@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495232}

base/containers/README.md

@@ -243,6 +243,10 @@ generally wastes a large amount of space (an Android analysis revealed more
 than 2.5MB wasted space from deque alone, resulting in some optimizations).
 libstdc++ uses an intermediate-size 512 byte buffer.
 
+Microsoft's implementation never shrinks the deque capacity, so the capacity
+will always be the maximum number of elements ever contained. libstdc++
+deallocates blocks as they are freed. libc++ keeps up to two empty blocks.
+
 ### base::circular_deque and base::queue
 
 A deque implemented as a circular buffer in an array. The underlying array will
@@ -251,9 +255,9 @@ around. The items will wrap around the underlying buffer so the storage will
 not be contiguous, but fast random access iterators are still possible.
 
 When the underlying buffer is filled, it will be reallocated and the constents
-moved (like a `std::vector`). As a result, iterators are not stable across
-mutations. Like a vector, it will not shrink its underlying storage. Consider
-calling `shrink_to_fit` if you delete many items that you don't plan to re-add.
+moved (like a `std::vector`). The underlying buffer will also be shrunk if
+there is too much wasted space. As a result, iterators are not stable across
+mutations.
 
 ## Appendix
 

base/containers/circular_deque.h

@@ -85,7 +85,7 @@
 //   const_reverse_iterator crend() const;
 //
 // Memory management:
-//   void reserve(size_t);
+//   void reserve(size_t);  // SEE IMPLEMENTATION FOR SOME GOTCHAS.
 //   size_t capacity() const;
 //   void shrink_to_fit();
 //
@@ -116,6 +116,12 @@ class circular_deque;
 
 namespace internal {
 
+// Start allocating nonempty buffers with this many entries. This is the
+// external capacity so the internal buffer will be one larger (= 4) which is
+// more even for the allocator. See the descriptions of internal vs. external
+// capacity on the comment above the buffer_ variable below.
+constexpr size_t kCircularBufferInitialCapacity = 3;
+
 template <typename T>
 class circular_deque_const_iterator {
  public:
@@ -522,23 +528,32 @@ class circular_deque {
   // ---------------------------------------------------------------------------
   // Memory management.
 
+  // IMPORTANT NOTE ON reserve(...): This class implements auto-shrinking of
+  // the buffer when elements are deleted and there is "too much" wasted space.
+  // So if you call reserve() with a large size in anticipation of pushing many
+  // elements, but pop an element before the queue is full, the capacity you
+  // reserved may be lost.
+  //
+  // As a result, it's only worthwhile to call reserve() when you're adding
+  // many things at once with no intermediate operations.
   void reserve(size_type new_capacity) {
     if (new_capacity > capacity())
-      ExpandCapacityTo(new_capacity + 1);
+      SetCapacityTo(new_capacity);
   }
+
   size_type capacity() const {
     // One item is wasted to indicate end().
     return buffer_.capacity() == 0 ? 0 : buffer_.capacity() - 1;
   }
+
   void shrink_to_fit() {
     if (empty()) {
       // Optimize empty case to really delete everything if there was
       // something.
       if (buffer_.capacity())
         buffer_ = VectorBuffer();
     } else {
-      // One item is wasted to indicate end().
-      ExpandCapacityTo(size() + 1);
+      SetCapacityTo(size());
     }
   }
 
@@ -563,7 +578,9 @@ class circular_deque {
 
   // When reducing size, the elements are deleted from the end. When expanding
   // size, elements are added to the end with |value| or the default
-  // constructed version.
+  // constructed version. Even when using resize(count) to shrink, a default
+  // constructor is required for the code to compile, even though it will not
+  // be called.
   //
   // There are two versions rather than using a default value to avoid
   // creating a temporary when shrinking (when it's not needed). Plus if
@@ -581,11 +598,11 @@ class circular_deque {
       while (size() < count)
         emplace_back();
     } else if (count < size()) {
-      // This doesn't resize the storage.
-      // TODO(brettw) revisit this decision (change below version also).
       size_t new_end = (begin_ + count) % buffer_.capacity();
       DestructRange(new_end, end_);
       end_ = new_end;
+
+      ShrinkCapacityIfNecessary();
     }
     IncrementGeneration();
   }
@@ -599,6 +616,8 @@ class circular_deque {
       size_t new_end = (begin_ + count) % buffer_.capacity();
       DestructRange(new_end, end_);
       end_ = new_end;
+
+      ShrinkCapacityIfNecessary();
     }
     IncrementGeneration();
   }
@@ -656,6 +675,8 @@ class circular_deque {
     if (begin_ == buffer_.capacity())
       begin_ = 0;
 
+    ShrinkCapacityIfNecessary();
+
     // Technically popping will not invalidate any iterators since the
     // underlying buffer will be stable. But in the future we may want to add a
     // feature that resizes the buffer smaller if there is too much wasted
@@ -670,6 +691,8 @@ class circular_deque {
       end_--;
     buffer_.DestructRange(&buffer_[end_], &buffer_[end_ + 1]);
 
+    ShrinkCapacityIfNecessary();
+
     // See pop_front comment about why this is here.
     IncrementGeneration();
   }
@@ -724,29 +747,48 @@ class circular_deque {
 
   // Expands the buffer size. This assumes the size is larger than the
   // number of elements in the vector (it won't call delete on anything).
-  // Note the capacity passed here will be one larger than the "publicly
-  // exposed capacity" to account for the unused end element.
-  void ExpandCapacityTo(size_t new_capacity) {
-    VectorBuffer new_buffer(new_capacity);
+  void SetCapacityTo(size_t new_capacity) {
+    // Use the capacity + 1 as the internal buffer size to differentiate
+    // empty and full (see definition of buffer_ below).
+    VectorBuffer new_buffer(new_capacity + 1);
     MoveBuffer(buffer_, begin_, end_, &new_buffer, &begin_, &end_);
     buffer_ = std::move(new_buffer);
   }
   void ExpandCapacityIfNecessary(size_t additional_elts) {
-    // Capacity is internal capacity, which is one extra.
-    size_t min_new_capacity = size() + additional_elts + 1;
-    if (buffer_.capacity() >= min_new_capacity)
+    size_t min_new_capacity = size() + additional_elts;
+    if (capacity() >= min_new_capacity)
       return;  // Already enough room.
 
-    // Start allocating nonempty buffers with this many entries.
-    constexpr size_t min_slots = 4;
-    min_new_capacity = std::max(min_new_capacity, min_slots);
+    min_new_capacity =
+        std::max(min_new_capacity, internal::kCircularBufferInitialCapacity);
 
     // std::vector always grows by at least 50%. WTF::Deque grows by at least
     // 25%. We expect queue workloads to generally stay at a similar size and
     // grow less than a vector might, so use 25%.
-    size_t new_capacity = std::max(
-        min_new_capacity, buffer_.capacity() + buffer_.capacity() / 4 + 1);
-    ExpandCapacityTo(new_capacity);
+    size_t new_capacity =
+        std::max(min_new_capacity, capacity() + capacity() / 4);
+    SetCapacityTo(new_capacity);
+  }
+
+  void ShrinkCapacityIfNecessary() {
+    // Don't auto-shrink below this size.
+    if (capacity() <= internal::kCircularBufferInitialCapacity)
+      return;
+
+    // Shrink when 100% of the size() is wasted.
+    size_t sz = size();
+    size_t empty_spaces = capacity() - sz;
+    if (empty_spaces < sz)
+      return;
+
+    // Leave 1/4 the size as free capacity, not going below the initial
+    // capacity.
+    size_t new_capacity =
+        std::max(internal::kCircularBufferInitialCapacity, sz + sz / 4);
+    if (new_capacity < capacity()) {
+      // Count extra item to convert to internal capacity.
+      SetCapacityTo(new_capacity);
+    }
   }
 
   // Backend for clear() but does not resize the internal buffer.
@@ -799,9 +841,16 @@ class circular_deque {
   void IncrementGeneration() {}
 #endif
 
-  // Danger, the buffer_.capacity() is capacity() + 1 since there is an
-  // extra item to indicate the end. Container internal code will want to use
-  // buffer_.capacity() for offset computations.
+  // Danger, the buffer_.capacity() is the "internal capacity" which is
+  // capacity() + 1 since there is an extra item to indicate the end. Otherwise
+  // being completely empty and completely full are indistinguishable (begin ==
+  // end). We could add a separate flag to avoid it, but that adds significant
+  // extra complexity since every computation will have to check for it. Always
+  // keeping one extra unused element in the buffer makes iterator computations
+  // much simpler.
+  //
+  // Container internal code will want to use buffer_.capacity() for offset
+  // computations rather than capacity().
   VectorBuffer buffer_;
   size_type begin_ = 0;
   size_type end_ = 0;

base/containers/circular_deque_unittest.cc

@@ -498,25 +498,38 @@ TEST(CircularDeque, CapacityReserveShrink) {
   EXPECT_EQ(new_capacity, q.size());
   EXPECT_EQ(new_capacity, q.capacity());
 
-  // Shrinking doesn't resize capacity.
+  // Shrink to fit to a smaller size.
   size_t capacity_2 = new_capacity / 2;
   q.resize(capacity_2);
-  EXPECT_EQ(capacity_2, q.size());
-  EXPECT_EQ(new_capacity, q.capacity());
-
-  // Shrink to fit to that size.
   q.shrink_to_fit();
   EXPECT_EQ(capacity_2, q.size());
   EXPECT_EQ(capacity_2, q.capacity());
+}
+
+TEST(CircularDeque, CapacityAutoShrink) {
+  size_t big_size = 1000u;
+  circular_deque<int> q;
+  q.resize(big_size);
+
+  size_t big_capacity = q.capacity();
+
+  // Delete 3/4 of the items.
+  for (size_t i = 0; i < big_size / 4 * 3; i++)
+    q.pop_back();
+
+  // The capacity should have shrunk by deleting that many items.
+  size_t medium_capacity = q.capacity();
+  EXPECT_GT(big_capacity, medium_capacity);
+
+  // Using resize to shrink should keep some extra capacity.
+  q.resize(1);
+  EXPECT_LT(1u, q.capacity());
 
-  // Shrink to 0, should have the same capacity.
   q.resize(0);
-  EXPECT_EQ(0u, q.size());
-  EXPECT_EQ(capacity_2, q.capacity());
+  EXPECT_LT(0u, q.capacity());
 
-  // Shrinking to fit should give 0 capacity.
-  q.shrink_to_fit();
-  EXPECT_EQ(0u, q.size());
+  // Using clear() should delete everything.
+  q.clear();
   EXPECT_EQ(0u, q.capacity());
 }
 
@@ -587,24 +600,31 @@ TEST(CircularDeque, ResizeDelete) {
   // The loops below assume the capacity will be set by resize().
   EXPECT_EQ(10u, q.capacity());
 
+  // Delete some via resize(). This is done so that the wasted items are
+  // still greater than the size() so that auto-shrinking is not triggered
+  // (which will mess up our destructor counting).
   counter = 0;
-  q.resize(5, DestructorCounter(&counter));
-  // 5 deleted ones + the one temporary in the resize() call.
-  EXPECT_EQ(6, counter);
-
-  // Cycle through some items so the 5 remaining items will cross the boundary
-  // in the 11-item buffer (one more than the capacity).
+  q.resize(8, DestructorCounter(&counter));
+  // 2 deleted ones + the one temporary in the resize() call.
+  EXPECT_EQ(3, counter);
+
+  // Cycle through some items so two items will cross the boundary in the
+  // 11-item buffer (one more than the capacity).
+  //   Before: x x x x x x x x . . .
+  //   After:  x . . . x x x x x x x
   counter = 0;
-  for (int i = 0; i < 7; i++) {
+  for (int i = 0; i < 4; i++) {
     q.emplace_back(&counter);
     q.pop_front();
   }
-  EXPECT_EQ(7, counter);  // Loop should have deleted 7 items.
+  EXPECT_EQ(4, counter);  // Loop should have deleted 7 items.
 
+  // Delete two items with resize, these should be on either side of the
+  // buffer wrap point.
   counter = 0;
-  q.resize(1, DestructorCounter(&counter));
-  // 4 deleted ones + the one temporary in the resize() call.
-  EXPECT_EQ(5, counter);
+  q.resize(6, DestructorCounter(&counter));
+  // 2 deleted ones + the one temporary in the resize() call.
+  EXPECT_EQ(3, counter);
 }
 
 /*