[libcxx-commits] [libcxx] [libc++] Optimize basic_string::append(Iter, Iter) (PR #169794)

[Louis Dionne via libcxx-commits]

================
@@ -1440,24 +1440,51 @@ public:
   template <class _ForwardIterator, __enable_if_t<__has_forward_iterator_category<_ForwardIterator>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string&
   append(_ForwardIterator __first, _ForwardIterator __last) {
-    size_type __sz  = size();
-    size_type __cap = capacity();
-    size_type __n   = static_cast<size_type>(std::distance(__first, __last));
+    const size_type __size = size();
+    const size_type __cap  = capacity();
+    const size_type __n    = static_cast<size_type>(std::distance(__first, __last));
     if (__n == 0)
       return *this;
 
-    if (__string_is_trivial_iterator_v<_ForwardIterator> && !__addr_in_range(*__first)) {
-      if (__cap - __sz < __n)
-        __grow_by_without_replace(__cap, __sz + __n - __cap, __sz, __sz, 0);
-      __annotate_increase(__n);
-      auto __end = __copy_non_overlapping_range(__first, __last, std::__to_address(__get_pointer() + __sz));
-      traits_type::assign(*__end, value_type());
-      __set_size(__sz + __n);
-      return *this;
+    __annotate_delete();
+    auto __asan_guard = std::__make_scope_guard(__annotate_new_size(*this));
+
+    if (__n > __cap - __size) {
+      __long __buffer  = __allocate_long_buffer(__alloc_, __get_amortized_growth_capacity(__size + __n));
+      __buffer.__size_ = __size + __n;
----------------
ldionne wrote:

Semantically, that's what we have:

```suggestion
      const size_type __new_size = __size + __n;
      const size_type __new_capacity = __get_amortized_growth_capacity(__new_size);
      __long __buffer  = __allocate_long_buffer(__alloc_, __new_size, __new_capacity);
```

It might not be necessary to actually name `__new_size`, but I think it would make sense for the API to allow us to write this.

The other way I can see to achieve the same result without losing the fact that we're doing amortized growth at the call site would be:

```c++
struct __amortized_growth_strategy_t {};
__long __buffer  = __allocate_long_buffer(__alloc_, __size + __n, __amortized_growth_strategy_t{});
```

and then

```c++
_LIBCPP_HIDE_FROM_ABI static _LIBCPP_CONSTEXPR_SINCE_CXX20 __long
  __allocate_long_buffer(_Allocator& __alloc, size_type __size, __amortized_growth_strategy_t) {
    _LIBCPP_ASSERT_INTERNAL(!__fits_in_sso(__size),
                            "Trying to allocate long buffer for a size what would fit into the small buffer");
    size_type const __capacity = has-amortized-growth-strategy ? __get_amortized_growth_capacity(__size) : __size;
    auto __buffer = std::__allocate_at_least(__alloc, __align_allocation_size(__capacity));

    if (__libcpp_is_constant_evaluated()) {
      for (size_type __i = 0; __i != __buffer.count; ++__i)
        std::__construct_at(std::addressof(__buffer.ptr[__i]));
    }

    return __long(__buffer, __size);
  }
```


https://github.com/llvm/llvm-project/pull/169794