[PATCH] D90884: [SmallVector] Add a default small size.

[Sean Silva via Phabricator via llvm-commits]

silvas created this revision.
Herald added subscribers: llvm-commits, dexonsmith.
Herald added a project: LLVM.
silvas requested review of this revision.

I find that I frequently set a fairly mindless small size (I tend to randomly choose "6").  This patch add a principled default small size to SmallVector.

The policy for the default small size is to guarantee that:

  sizeof(SmallVector<T>) <=
    kDefaultSmallSizeHeaderSizeMultiplier * sizeof(SmallVectorImpl<T>)

I have set `kDefaultSmallSizeHeaderSizeMultiplier == 3` based on a
spot-check of various SmallVector's in the codebase.
That is, `SmallVector<T>` will never be more than 3 vector headers.

This tends to match my intuition: depending on the element size, you
want to be careful about your small size -- basically we just want to
ensure not too much stack growth (or heap allocation growth for the
heap-allocated case). Also, looking back through llvm-dev, it looks
like Reid suggested something similar in
this thread <https://groups.google.com/g/llvm-dev/c/q1OyHZy8KVc/m/1l_AasOLBAAJ>
though his suggestion corresponds to
`kDefaultSmallSizeHeaderSizeMultiplier == 2` (happy to change that).

This default also tends to do the right thing in
`SmallVector<SmallVector<T>>` sorts of situations (also mentioned in
that thread as problematic). For these, the outer SmallVector will not
amplify the sizes of the inner SmallVectors. Of course, users can do
`SmallVector<SmallVector<T, 0>>` to indicate that the inner
SmallVector's should have no inline elements, and the outer SmallVector
then be able to fit some inner SmallVector's inline.

Finally, forgetting the small size is a frequent extra edit/rebuild
iteration for me. This avoids that which makes me more productive.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90884

Files:
  llvm/include/llvm/ADT/SmallVector.h
  llvm/unittests/ADT/SmallVectorTest.cpp


Index: llvm/unittests/ADT/SmallVectorTest.cpp
===================================================================
--- llvm/unittests/ADT/SmallVectorTest.cpp
+++ llvm/unittests/ADT/SmallVectorTest.cpp
@@ -1029,4 +1029,29 @@
   EXPECT_TRUE(makeArrayRef(V2).equals({4, 5, 3, 2}));
 }
 
+struct BigElementType {
+  char Arr[10000] = {0};
+};
+typedef ::testing::Types<SmallVector<int8_t>, SmallVector<int16_t>,
+                         SmallVector<int32_t>, SmallVector<int64_t>,
+                         SmallVector<void *>, SmallVector<BigElementType>>
+    SmallVectorDefaultSmallSizeTestTypes;
+// Test fixture class
+template <typename VectorT>
+class SmallVectorDefaultSmallSizeTest : public testing::Test {
+protected:
+  VectorT theVector;
+};
+
+TYPED_TEST_CASE(SmallVectorDefaultSmallSizeTest,
+                SmallVectorDefaultSmallSizeTestTypes);
+
+TYPED_TEST(SmallVectorDefaultSmallSizeTest, DefaultSmallSize) {
+  using ValueT = typename decltype(this->theVector)::value_type;
+  static_assert(sizeof(this->theVector) <=
+                    kDefaultSmallSizeHeaderSizeMultiplier *
+                        sizeof(SmallVectorImpl<ValueT>),
+                "");
+}
+
 } // end namespace
Index: llvm/include/llvm/ADT/SmallVector.h
===================================================================
--- llvm/include/llvm/ADT/SmallVector.h
+++ llvm/include/llvm/ADT/SmallVector.h
@@ -892,6 +892,21 @@
 /// well-defined.
 template <typename T> struct alignas(T) SmallVectorStorage<T, 0> {};
 
+// Parameter controlling the default small size for SmallVector.
+//
+// We guarantee that a default-small-sized SmallVector has sizeof no more than
+// kDefaultSmallSizeHeaderSizeMultiplier times bigger than just the header.
+//
+// This prevents unexpected variation based on `sizeof(value_type)`.
+constexpr size_t kDefaultSmallSizeHeaderSizeMultiplier = 3;
+template <typename ElementTy> constexpr size_t calculateDefaultSmallSize() {
+  // Discount the size of the header itself when calculating the maximum inline
+  // bytes, so subtract one fromthe multiplier.
+  size_t MaxInlineBytes = (kDefaultSmallSizeHeaderSizeMultiplier - 1) *
+                          sizeof(SmallVectorImpl<ElementTy>);
+  return MaxInlineBytes / sizeof(ElementTy);
+}
+
 /// This is a 'vector' (really, a variable-sized array), optimized
 /// for the case when the array is small.  It contains some number of elements
 /// in-place, which allows it to avoid heap allocation when the actual number of
@@ -900,7 +915,7 @@
 ///
 /// Note that this does not attempt to be exception safe.
 ///
-template <typename T, unsigned N>
+template <typename T, unsigned N = calculateDefaultSmallSize<T>()>
 class LLVM_GSL_OWNER SmallVector : public SmallVectorImpl<T>,
                                    SmallVectorStorage<T, N> {
 public:


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D90884.303259.patch
Type: text/x-patch
Size: 2818 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201105/5288e4de/attachment.bin>