[WIP][ADT] Avoid slow size queries on append

[kuhar]

Only calculate the size and reserve upfront when the input iterators support cheap (O(1)) distance.

This is a follow up to:
#136066 (comment).

[kuhar]

@nikic Would you be able to see if this helps with compile times?

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: Jakub Kuderski (kuhar)

Changes

Only calculate the size and reserve upfront when the input iterators support cheap (O(1)) distance.

This is a follow up to:
#136066 (comment).

---

Full diff: https://github.com/llvm/llvm-project/pull/136365.diff

1 Files Affected:

• (modified) llvm/include/llvm/ADT/SmallVector.h (+48-13)

diff --git a/llvm/include/llvm/ADT/SmallVector.h b/llvm/include/llvm/ADT/SmallVector.h
index bd3e887e36bce..104a3aaf79418 100644
--- a/llvm/include/llvm/ADT/SmallVector.h
+++ b/llvm/include/llvm/ADT/SmallVector.h
@@ -36,10 +36,18 @@ template <typename T> class ArrayRef;
 
 template <typename IteratorT> class iterator_range;
 
-template <class Iterator>
-using EnableIfConvertibleToInputIterator = std::enable_if_t<std::is_convertible<
+namespace detail {
+template <typename Iterator, typename IteratorCategory>
+inline constexpr bool IsOfIteratorCategory = std::is_convertible_v<
     typename std::iterator_traits<Iterator>::iterator_category,
-    std::input_iterator_tag>::value>;
+    IteratorCategory>;
+
+template <typename Iterator>
+using EnableIfConvertibleToInputIterator =
+    std::enable_if_t<std::is_convertible_v<
+        typename std::iterator_traits<Iterator>::iterator_category,
+        std::input_iterator_tag>>;
+} // namespace detail
 
 /// This is all the stuff common to all SmallVectors.
 ///
@@ -679,13 +687,37 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
   void swap(SmallVectorImpl &RHS);
 
   /// Add the specified range to the end of the SmallVector.
-  template <typename ItTy, typename = EnableIfConvertibleToInputIterator<ItTy>>
+  template <typename ItTy,
+            typename = detail::EnableIfConvertibleToInputIterator<ItTy>>
   void append(ItTy in_start, ItTy in_end) {
     this->assertSafeToAddRange(in_start, in_end);
-    size_type NumInputs = std::distance(in_start, in_end);
-    this->reserve(this->size() + NumInputs);
-    this->uninitialized_copy(in_start, in_end, this->end());
-    this->set_size(this->size() + NumInputs);
+    if constexpr (detail::IsOfIteratorCategory<
+                      ItTy, std::random_access_iterator_tag>) {
+      // Only reserve the required extra size upfront when the size calculation
+      // is guaranteed to be O(1).
+      size_type NumInputs = std::distance(in_start, in_end);
+      this->reserve(this->size() + NumInputs);
+      this->uninitialized_copy(in_start, in_end, this->end());
+      this->set_size(this->size() + NumInputs);
+    } else {
+      // Otherwise, append using `in_end` as the sentinel and reserve more space
+      // as necessary.
+      for (ItTy It = in_start; It != in_end;) {
+        iterator Dest = this->end();
+        size_type InitialSize = this->size();
+        size_type ExtraCap = this->capacity() - InitialSize;
+        size_type NumCopied = 0;
+        for (; NumCopied != ExtraCap && It != in_end; ++It, ++NumCopied) {
+          ::new ((void *)(Dest + NumCopied)) T(*It);
+        }
+        size_type NewSize = InitialSize + NumCopied;
+        this->set_size(NewSize);
+
+        if (It != in_end) {
+          this->reserve(NewSize + 1);
+        }
+      }
+    }
   }
 
   /// Append \p NumInputs copies of \p Elt to the end.
@@ -720,7 +752,8 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
   // FIXME: Consider assigning over existing elements, rather than clearing &
   // re-initializing them - for all assign(...) variants.
 
-  template <typename ItTy, typename = EnableIfConvertibleToInputIterator<ItTy>>
+  template <typename ItTy,
+            typename = detail::EnableIfConvertibleToInputIterator<ItTy>>
   void assign(ItTy in_start, ItTy in_end) {
     this->assertSafeToReferenceAfterClear(in_start, in_end);
     clear();
@@ -871,7 +904,8 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
     return I;
   }
 
-  template <typename ItTy, typename = EnableIfConvertibleToInputIterator<ItTy>>
+  template <typename ItTy,
+            typename = detail::EnableIfConvertibleToInputIterator<ItTy>>
   iterator insert(iterator I, ItTy From, ItTy To) {
     // Convert iterator to elt# to avoid invalidating iterator when we reserve()
     size_t InsertElt = I - this->begin();
@@ -887,8 +921,8 @@ class SmallVectorImpl : public SmallVectorTemplateBase<T> {
     this->assertSafeToAddRange(From, To);
 
     size_t NumToInsert = std::distance(From, To);
-
-    // Ensure there is enough space.
+    // Ensure there is enough space so that we do not have to re-allocate mid
+    // insertion.
     reserve(this->size() + NumToInsert);
 
     // Uninvalidate the iterator.
@@ -1212,7 +1246,8 @@ class LLVM_GSL_OWNER SmallVector : public SmallVectorImpl<T>,
     this->assign(Size, Value);
   }
 
-  template <typename ItTy, typename = EnableIfConvertibleToInputIterator<ItTy>>
+  template <typename ItTy,
+            typename = detail::EnableIfConvertibleToInputIterator<ItTy>>
   SmallVector(ItTy S, ItTy E) : SmallVectorImpl<T>(N) {
     this->append(S, E);
   }

[kazutakahirata]

Thank you for working on this!

[kazutakahirata]

Could we use the newly introduced IsOfIteratorCategory here?

[kuhar]

Actually no because of SFINAE weirdness...

[kazutakahirata]

Are you going to address this call to std::distance? If we have an input iterator that is not a forward iterator, then we would be iterating the range twice.

[kuhar]

This is only invoked when we insert in the middle of the vector -- I'm not sure how much we care about this. We could potentially first append and then when we know size do a rotate, but I'm not sure it's worth it

[kuhar]

Unless I misread the code...

[kuhar]

I think this is what the if statement above already handles -- it's not just the empty case, unlike what the comment claims.

[nikic]

@nikic Would you be able to see if this helps with compile times?

The change itself has a very small negative effect: https://llvm-compile-time-tracker.com/compare.php?from=f696508b38d1958aac5992a98a90fe6e773e8709&to=a69d10e1ebd2e99155aa893ee9b337bf7b3aadd3&stat=instructions:u

If I restore the append_range use in BFI I still get a regression: https://llvm-compile-time-tracker.com/compare.php?from=a69d10e1ebd2e99155aa893ee9b337bf7b3aadd3&to=2e17e3511a77e2f054c9cb215d3156f497c39ac0&stat=instructions:u

It would not have expected the regression to still be there with this change, so I suspect something isn't right...

[kuhar]

@nikic How can I reproduce your compile time benchmarks locally, or find some close enough proxy? Would running opt on a large .bc file under perf be close enough? I think this should be the script used to produce the data: https://github.com/nikic/llvm-compile-time-tracker/blob/master/timeit.sh.

[nikic]

@nikic How can I reproduce your compile time benchmarks locally, or find some close enough proxy? Would running opt on a large .bc file under perf be close enough? I think this should be the script used to produce the data: https://github.com/nikic/llvm-compile-time-tracker/blob/master/timeit.sh.

See https://llvm-compile-time-tracker.com/about.php for some information on reproducing. I usually use callgrind for local profiling of small test cases because the results are stable. sqlite3.c is often a good one. Though in this case I'd expect that pretty much any input will work.