[libcxx] Optimize __split_buffer::__relocate by using size-based __set_valid_range

[liza371]

In __split_buffer_size_layout::__relocate, the valid range was previously updated using __set_valid_range(__new_begin, end()). The size-based layout also used the pointer-based __set_valid_range(). This led to a redundent calculation due to the call to __set_sentinel() in the pointer-based method.

end()                     => __begin_ + __size_
__set_sentinel(__new_end) => __size_ = __new_end - __begin_

__size_ = (__begin_ + __size_) - (__begin_)
__size_ = __size_

Since the new size is already known (__n), we can directly call the (pointer, size_type) overload: __set_valid_range(__new_begin, __n). This avoids the redundant pointer arithmetic.

This optimization also reduces some of the binary size bloat Chromium has seen with the introduction of the size-based vector representation.

For reference, here is a breakdown of the binary size change from a
recent libc++ roll:
https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchromium-binary-size-trybot-results%2Fandroid-binary-size%2F2026%2F07%2F13%2F2825561%2FTrichrome32.ssargs.sizediff&group_by=template

Here is a breakdown of the binary size change with the size-based vector turned off:

https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchrome-supersize%2Foneoffs%2Fb16984ce99c702355a5b2b4c52574e82cec41fb9_9f0668506b95d84647600cd74b220f49e0c399a8.sizediff&group_by=template

Here is a breakdown of the binary size changes with the changes in this patch:

https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchrome-supersize%2Foneoffs%2Fb16984ce99c702355a5b2b4c52574e82cec41fb9_cf9629cceca1c32c3ca9ce09d403f132477a7c35.sizediff&group_by=template

Co-Authored-By: Christopher Di Bella cjdb@google.com

[llvmorg-github-actions]

@llvm/pr-subscribers-libcxx

Author: Liza Burakova (liza371)

Changes

TL;DR: Chromium saw a large binary increase with the new size-based
vector representation, and this PR introduces optimizations to reduce
it.

This PR introduces the following changes:

1. inline vector __relocate
2. directly update split_buffer's __begin and __size via arithemetic in __split_buffer_size_layout::__relocate instead of recalculating using __set_valid_range
3. replace the lambda-based if_likely_else construct in emplace_back
   with a direct if (size() == capacity()) [[unlikely]]

Christopher Di Bella wrote these patches, I am submitting them on his
behalf while he is out so that Chromium can pick up these changes from
upstream and reduce the binary size increase we are seeing.

I understand that there was a discussion on PR #155330 around keeping
if_likely_else in emplace_back since it could lead to a codegen
regression, I think this change is small enough that we can investigate
the potential regression in this change, but I can also move the
emplace_back change into its own PR if that is preferable.

The recent introduction of a size-based vector representation caused a
large binary size increase of 550kb on arm32 Chromium android builds,
and a 5mb increase on arm64 builds.

For reference, here is a breakdown of the binary size change from a
recent libc++ roll:
https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchromium-binary-size-trybot-results%2Fandroid-binary-size%2F2026%2F07%2F13%2F2825561%2FTrichrome32.ssargs.sizediff&group_by=template

This breakdown should be publicly available, please let me know if there
are access restrictions. The above breakdown is also specifically
grouped by template, and as you can see the biggest increase is in
std::vector and std::split_buffer.

We found that with the following changes we saw a decrease in binary
size. Here is a breakdown of the binary size diff with the changes in
this PR:
https://chrome-supersize.firebaseapp.com/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchrome-supersize%2Foneoffs%2Fb16984ce99c702355a5b2b4c52574e82cec41fb9_8d9c63dff3faee22041b794137b16d8f6c3a5c6a.sizediff&group_by=template

Co-Authored-By: Christopher Di Bella <cjdb@google.com>

---

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

3 Files Affected:

• (modified) libcxx/include/__split_buffer (+3-1)
• (modified) libcxx/include/__vector/layout.h (+2-2)
• (modified) libcxx/include/__vector/vector.h (+9-38)

diff --git a/libcxx/include/__split_buffer b/libcxx/include/__split_buffer
index 59cee84536069..95c1afd45a247 100644
--- a/libcxx/include/__split_buffer
+++ b/libcxx/include/__split_buffer
@@ -416,7 +416,9 @@ public:
     auto __last      = __first + __n;
     std::__uninitialized_allocator_relocate(
         __alloc_, std::__to_address(__first), std::__to_address(__last), std::__to_address(__new_begin));
-    __set_valid_range(__new_begin, end());
+    __begin_ = __new_begin;
+    __size_ += __n;
+
     __n = 0;
 
     __swap_layouts(__first, __n, __capacity);
diff --git a/libcxx/include/__vector/layout.h b/libcxx/include/__vector/layout.h
index af03556dc2636..f64b6d79e220f 100644
--- a/libcxx/include/__vector/layout.h
+++ b/libcxx/include/__vector/layout.h
@@ -289,7 +289,7 @@ __vector_layout<_Tp, _Alloc>::__move_assign_without_allocator(__vector_layout& _
 template <class _Tp, class _Alloc>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void __vector_layout<_Tp, _Alloc>::__relocate(_SplitBuffer& __buffer) {
   __annotate_delete();
-  __buffer.__relocate(__begin_, __size_, __capacity_);
+  [[clang::always_inline]] __buffer.__relocate(__begin_, __size_, __capacity_);
   __annotate_new(__size_);
 }
 
@@ -406,7 +406,7 @@ __vector_layout<_Tp, _Alloc>::__move_assign_without_allocator(__vector_layout& _
 template <class _Tp, class _Alloc>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void __vector_layout<_Tp, _Alloc>::__relocate(_SplitBuffer& __buffer) {
   __annotate_delete();
-  __buffer.__relocate(__begin_, __end_, __capacity_);
+  [[clang::always_inline]] __buffer.__relocate(__begin_, __end_, __capacity_);
   __annotate_new(__size());
 }
 
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 5d2ef8fc2a583..1d926b5d7a715 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -466,7 +466,15 @@ class _LIBCPP_WARN_UNUSED vector {
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void push_back(value_type&& __x) { emplace_back(std::move(__x)); }
 
   template <class... _Args>
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI __emplace_back_result_t emplace_back(_Args&&... __args);
+  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI reference emplace_back(_Args&&... __args) {
+    if (size() == capacity()) [[unlikely]] {
+      __emplace_back_slow_path(std::forward<_Args>(__args)...);
+    } else {
+      __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
+    }
+
+    return static_cast<__emplace_back_result_t>(back());
+  }
 
   template <class... _Args>
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __emplace_back_assume_capacity(_Args&&... __args) {
@@ -1054,43 +1062,6 @@ vector<_Tp, _Allocator>::__emplace_back_slow_path(_Args&&... __args) {
   return __end;
 }
 
-// This makes the compiler inline `__else()` if `__cond` is known to be false. Currently LLVM doesn't do that without
-// the `__builtin_constant_p`, since it considers `__else` unlikely even through it's known to be run.
-// See https://llvm.org/PR154292
-template <class _If, class _Else>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void __if_likely_else(bool __cond, _If __if, _Else __else) {
-  if (__builtin_constant_p(__cond)) {
-    if (__cond)
-      __if();
-    else
-      __else();
-  } else {
-    if (__cond) [[__likely__]]
-      __if();
-    else
-      __else();
-  }
-}
-
-template <class _Tp, class _Alloc>
-template <class... _Args>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI typename vector<_Tp, _Alloc>::__emplace_back_result_t
-vector<_Tp, _Alloc>::emplace_back(_Args&&... __args) {
-  pointer __end = __layout_.__end_ptr();
-  std::__if_likely_else(
-      size() != capacity(),
-      [&] {
-        __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
-        ++__end;
-      },
-      [&] { __end = __emplace_back_slow_path(std::forward<_Args>(__args)...); });
-
-  __layout_.__set_bound_using_pointer(__end);
-#if _LIBCPP_STD_VER >= 17
-  return back();
-#endif
-}
-
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 inline _LIBCPP_HIDE_FROM_ABI typename vector<_Tp, _Allocator>::iterator
 vector<_Tp, _Allocator>::erase(const_iterator __position) {

[philnik777]

This should really be split into its own patch. We've tried quite a bit to make this optimize properly, and this basically reverts that. That patch should probably include an analysis what actually prevents some optimization here.

[liza371]

Ok no problem, I reverted this fully for this patch and I'll work on a separate patch for this.

[philnik777]

Why does this change anything? Can't you use the size overload of __set_valid_range instead?

[liza371]

Yes you're right I can use the size overload. Admittedly I'm actually not entirely sure why this changes anything, I re-ran the binary size analysis tool I've been using with just this change to get a better sense of the true change, and it was a relatively small change.

I actually need to rerun this analysis one more time to make sure the chromium checkouts are correct for for each build its looking at, as I believe that this tool is now comparing against an older revision of chrome and an older revision of libc++ so I'm not getting accurate data.

Beyond that, though, I think using the size overload is a general improvement here because using the pointer overload the math is basically

end()                     => __begin_ + __size_
__set_sentinel(__new_end) => __size_ = __new_end - __begin_

so we end up with

__size_ = (__begin_ + __size_) - (__begin_)
__size_ = __size_

So I think that regardless of how my rerun of the binary size analysis turns out, it makes sense to at least update this to use the size overload __set_valid_range

[philnik777]

I probably don't mind this as an NFC change, but I feel like a patch about improving performance shouldn't touch an entirely different file unnecessarily. (Though, I would like to note that your analysis completely ignores the much more widely used stable ABI).

[liza371]

Alrighty so I just removed the inlining change altogether, and the PR description now includes a diff analysis that has just this patch. It's not as large of an improvement as your changes to emplace_back but it's still helpful imho.

[ldionne]

LGTM pending CI.