[libc++] Mark __{emplace,push}_back_slow_path as noinline

[philnik777]

These are almost certainly intended to not be inlined. This significantly reduces code size when push_back and emplace_back are used heavily.

Fixes #94360

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

These are almost ceratinly intended to not be inlined. This significantly reduces code size when push_back and emplace_back are used heavily.

Fixes #94360

---

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

1 Files Affected:

• (modified) libcxx/include/vector (+3-2)

diff --git a/libcxx/include/vector b/libcxx/include/vector
index cbfc2cefa1fd9..c542b3e70cdd1 100644
--- a/libcxx/include/vector
+++ b/libcxx/include/vector
@@ -844,10 +844,11 @@ private:
   }
 
   template <class _Up>
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI inline pointer __push_back_slow_path(_Up&& __x);
+  _LIBCPP_NOINLINE _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI inline pointer __push_back_slow_path(_Up&& __x);
 
   template <class... _Args>
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI inline pointer __emplace_back_slow_path(_Args&&... __args);
+  _LIBCPP_NOINLINE _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI inline pointer
+  __emplace_back_slow_path(_Args&&... __args);
 
   // The following functions are no-ops outside of AddressSanitizer mode.
   // We call annotations for every allocator, unless explicitly disabled.

[gerben-stavenga]

This unfortunately will hurt performance greatly (think 5x) on a simple loop like
for(int i =0; i< n; i++) v.push_back(i);

[gerben-stavenga]

Furthermore, I don't understand the philosophy of push_back_slow_path in this case. The act of constructing a new type at the end of the vector happens regardless if the buffer needs to grow or not. Why would you duplicate the construction code? You just want to grow the buffer, which is the same between emplace_back or push_back.

This way of tailcalling slow paths only make sense if the function is outline in the first place, where it potentially prevents stack frames. But push_back is made to be inlined.

[EricWF]

My tendency here is to trust the compiler. The example output on the code presented in the bug may seem silly, but why wouldn't the compiler inline there? It's the only function in a translation unit, and it contains a couple of lines. I wonder under what circumstances the compiler chooses to not inline the code.

However, when we prevent the compiler from inlining, we prevent it from optimizing away dead code.

Before you change, this silly example is optimized away.

void foo() {
    std::vector<int> v;
    v.push_back(42);
}
// foo(): # @foo()
//   ret

Now it generates

foo(): # @foo()
  push rbx
  sub rsp, 48
  xorps xmm0, xmm0
  movaps xmmword ptr [rsp + 16], xmm0
  mov qword ptr [rsp + 32], 0
  mov dword ptr [rsp + 12], 42
  lea rdi, [rsp + 16]
  lea rsi, [rsp + 12]
  call int* std::__2::vector<int, std::__2::allocator<int> >::__push_back_slow_path<int>(int&&)
  mov rdi, qword ptr [rsp + 16]
  test rdi, rdi
  je .LBB0_3
  mov qword ptr [rsp + 24], rdi
  call operator delete(void*)@PLT
.LBB0_3:
  add rsp, 48
  pop rbx
  ret
  mov rbx, rax
  mov rdi, qword ptr [rsp + 16]
  test rdi, rdi
  je .LBB0_6
  mov qword ptr [rsp + 24], rdi
  call operator delete(void*)@PLT
.LBB0_6:
  mov rdi, rbx
  call _Unwind_Resume@PLT
.L.str:
  .asciz "vector"

Use your discretion here. Forbidding the optimizer from inlining the slow path may be a good choice.
Here's a couple of observations:

• libstdc++ produces the same output roughly.
• We explicitly add inline to __push_back_slow_path, which here only acts an an inline hint to the compiler. We should 100% remove that, if we do then:
• Compiling with -Os gives the user the codegen they want, without affecting -O2/-O3

[gerben-stavenga]

My tendency here is to trust the compiler. The example output on the code presented in the bug may seem silly, but why wouldn't the compiler inline there? It's the only function in a translation unit, and it contains a couple of lines. I wonder under what circumstances the compiler chooses to not inline the code.

My point is that the code is wrongheaded. Either the compiler inlines everything and enormous bloat is caused, and if it decides not to inline performance in useful cases drops like a rock. The code should be written that the performance of the fast path is superior independent of the inline/outline decisions.

However, when we prevent the compiler from inlining, we prevent it from optimizing away dead code.

Before you change, this silly example is optimized away.

void foo() {
    std::vector<int> v;
    v.push_back(42);
}

Yes due to the special nature of the new/delete functions the compiler could do this with full inlining. I don't think the side effect free no-ops are that important. I think for stdlib cases outline functions could be annotated with some kind of alloc/realloc/free attributes that similar transformations could still happen, but in the end not all that important for actual code.

[EricWF]

My tendency here is to trust the compiler. The example output on the code presented in the bug may seem silly, but why wouldn't the compiler inline there? It's the only function in a translation unit, and it contains a couple of lines. I wonder under what circumstances the compiler chooses to not inline the code.

My point is that the code is wrongheaded. Either the compiler inlines everything and enormous bloat is caused, and if it decides not to inline performance in useful cases drops like a rock. The code should be written that the performance of the fast path is superior independent of the inline/outline decisions.

I agree, it would be nice if we could have it all.

Could you talk about the concrete problem that lead you here?

This conversation is lacking a grounding. It's easy to point at the "bloat", but why does "bloat" matter concretely to you?
Are you encountering production binaries where push_back is a problematically large portion of the final binary?

However, when we prevent the compiler from inlining, we prevent it from optimizing away dead code.
Before you change, this silly example is optimized away.

void foo() {
    std::vector<int> v;
    v.push_back(42);
}

Yes due to the special nature of the new/delete functions the compiler could do this with full inlining. I don't think the side effect free no-ops are that important. I think for stdlib cases outline functions could be annotated with some kind of alloc/realloc/free attributes that similar transformations could still happen, but in the end not all that important for actual code.

I think code like this appears in less contrived manners more than one might expect. The point of the example is to show how little the optimizer can do with such trivial code. If it can't optimize around that, then I expect it to do worse elsewhere.

Here's another less contrived example that compiles away:

static std::vector<int> foo(std::vector<int> const& LHS, std::vector<int> const& RHS)
{
  // Yes, this was originally a bug, but for some reason it causes the code to optimize better?
  std::vector<int> v(3);
 
  for (int i=0; i < 3; ++i)
    v.push_back(LHS[i] + RHS[i]);

 return v;
}

int main() {
    int sum = 42;
    for (auto x : foo({1, 2, 3}, {1, 2, 3})) {
        sum += x;
    }
    return sum;
}

[gerben-stavenga]

When someone optimizes some code one can equivalent ask "Code is 20% too slow, but how does that matter concretely to you?" Well, I can confidently say I'm not suffering terribly personally.

I'm not seeing it as you pasted it, but "return std::move(v);" does indeed do the trick. Never do RVO for vector, always std::move a vector.

https://godbolt.org/z/j4q5EcqK9

Anyway, I'm just pointing out what, to me at least, is obviously too much code generated for what push_back does. We do suffer from massive binaries and often it's a death by thousands cuts and a lot of code that just is too bloated. The c++ standard due to it's templated nature often contributes significantly. Now I think due to it's fundamental status in the ecosystem it's good to give code generation of standard lib functions a good inspection.

[EricWF]

@philnik777 Would you be open simply removing the inline here so that with -Os, the compiler doesn't inline it.

That way we don't have to add _LIBCPP_NOINLINE and pessimize other code?

I would LGTM that change in a heartbeat.

Otherwise, I think we should try to get some concrete performance numbers here.

[EricWF]

When someone optimizes some code one can equivalent ask "Code is 20% too slow, but how does that matter concretely to you?" Well, I can confidently say I'm not suffering terribly personally.

I'm not seeing it as you pasted it, but "return std::move(v);" does indeed do the trick. Never do RVO for vector, always std::move a vector.

https://godbolt.org/z/j4q5EcqK9

Anyway, I'm just pointing out what, to me at least, is obviously too much code generated for what push_back does. We do suffer from massive binaries and often it's a death by thousands cuts and a lot of code that just is too bloated. The c++ standard due to it's templated nature often contributes significantly. Now I think due to it's fundamental status in the ecosystem it's good to give code generation of standard lib functions a good inspection.

The reason I asked for a concrete 1st order problem caused by the codegen is because I don't think it is "obvious"
that there is "too much code generated". Why does the optimizer perform the inlining if it's obviously bad? Should we fix the inliner?

To take your example about code "being 20% too slow", I would have to refer to something like "It means my program handles 5% fewer queries a second" as my 1st order concern. It's too slow because it affects my programs throughput measurably.

Intuition has been a very poor tool in my experience benchmarking code, and I so I'm asking for something more in order to understand and help resolve your concern.

How did you first come to inspect the assembly for push_back?

[gerben-stavenga]

When someone optimizes some code one can equivalent ask "Code is 20% too slow, but how does that matter concretely to you?" Well, I can confidently say I'm not suffering terribly personally.
I'm not seeing it as you pasted it, but "return std::move(v);" does indeed do the trick. Never do RVO for vector, always std::move a vector.
https://godbolt.org/z/j4q5EcqK9
Anyway, I'm just pointing out what, to me at least, is obviously too much code generated for what push_back does. We do suffer from massive binaries and often it's a death by thousands cuts and a lot of code that just is too bloated. The c++ standard due to it's templated nature often contributes significantly. Now I think due to it's fundamental status in the ecosystem it's good to give code generation of standard lib functions a good inspection.

The reason I asked for a concrete 1st order problem caused by the codegen is because I don't think it is "obvious" that there is "too much code generated". Why does the optimizer perform the inlining if it's obviously bad? Should we fix the inliner?

I don't think of this as "a problem of the inliner". The code, as is, is written in a way that on a very fundamental level provides no good option to the compiler. Ie. either, one, inline everything and get good performance on the fast path or, two, outline the slow path and get terrible performance.

A simple

for (int i = 0; i < n; i++) x.push_back(i);

will be much slower. Because the fast path is suddenly much slower, due to the fact that the pointer to vector escapes. The compiler will store and reload size, because it can't prove the store of i doesn't overwrite the size variable.

All I'm saying is that a redesign of the library such that the potential non-inline fallback functions are static member functions that pass in the data by value and return by value. Now the compiler can choose to inline or outline functions at will and it won't affect performance at all.

To take your example about code "being 20% too slow", I would have to refer to something like "It means my program handles 5% fewer queries a second" as my 1st order concern. It's too slow because it affects my programs throughput measurably.

Intuition has been a very poor tool in my experience benchmarking code, and I so I'm asking for something more in order to understand and help resolve your concern.

How did you first come to inspect the assembly for push_back?

See above analysis of push_back performance concerns for reasons i inspect push_back.

I saw 5x degradation (with different versions of clang and -o2 vs -o3) due not inlining the throw_bad_length_error (hence this-pointer escapes hence function is 5x too slow).

[EricWF]

I feel like we're talking past each other. Best of luck.

[gerben-stavenga]

I did some code archeology, I see
6fe4e03
already introduced some of the optimizations i did at google on protobuf repeatedfield. I was investigating an older version of libc++ that we are using which misses this. Basically what I'm saying is that returning capacity as well and making sure that fallback functions are static so that this pointer never escapes, we can get basically perfect fast path code without having to do excessive inlining.

[cjdb]

I'm running through some internal benchmarks right now.

[ldionne]

We just discussed this issue with @philnik777 and we went through several attempted solutions. First, we wanted to make the code look like what follows, which is simplest from the readability point of view. By using [[unlikely]] on the "reallocate" branch, we wanted to tell the optimizer that it should keep the reserve() call outlined.

Unfortunately, this rewrite doesn't work because it doesn't support v.emplace_back(v.front()). To support that, we need to allocate a new buffer, emplace the element into it, and finally move the existing elements. There's just no way to reorganize things to simplify it. It's sad, cause it would have been a nice simplification.

diff --git a/libcxx/include/vector b/libcxx/include/vector
index bfbf1ea1cfc9..8d4e32866675 100644
--- a/libcxx/include/vector
+++ b/libcxx/include/vector
@@ -656,6 +656,7 @@ public:
     return this->__begin_ == this->__end_;
   }
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI size_type max_size() const _NOEXCEPT;
+  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __reserve_for_real(size_type __n);
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void reserve(size_type __n);
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void shrink_to_fit() _NOEXCEPT;
 
@@ -901,9 +902,6 @@ private:
   template <class _Up>
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI inline pointer __push_back_slow_path(_Up&& __x);
 
-  template <class... _Args>
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI inline pointer __emplace_back_slow_path(_Args&&... __args);
-
   // The following functions are no-ops outside of AddressSanitizer mode.
   // We call annotations for every allocator, unless explicitly disabled.
   //
@@ -1460,14 +1458,21 @@ vector<_Tp, _Allocator>::at(size_type __n) const {
   return this->__begin_[__n];
 }
 
+// This one assumes that we don't have enough capacity
+// It allows callers to inline the capacity check
+template <class _Tp, class _Allocator>
+_LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::__reserve_for_real(size_type __n) {
+  if (__n > max_size())
+    this->__throw_length_error();
+  allocator_type& __a = this->__alloc();
+  __split_buffer<value_type, allocator_type&> __v(__n, size(), __a);
+  __swap_out_circular_buffer(__v);
+}
+
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::reserve(size_type __n) {
   if (__n > capacity()) {
-    if (__n > max_size())
-      this->__throw_length_error();
-    allocator_type& __a = this->__alloc();
-    __split_buffer<value_type, allocator_type&> __v(__n, size(), __a);
-    __swap_out_circular_buffer(__v);
+    __reserve_for_real(__n);
   }
 }
 
@@ -1529,19 +1534,6 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline _LIBCPP_HIDE_FROM_ABI void vector<_Tp, _All
   this->__end_ = __end;
 }
 
-template <class _Tp, class _Allocator>
-template <class... _Args>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::pointer
-vector<_Tp, _Allocator>::__emplace_back_slow_path(_Args&&... __args) {
-  allocator_type& __a = this->__alloc();
-  __split_buffer<value_type, allocator_type&> __v(__recommend(size() + 1), size(), __a);
-  //    __v.emplace_back(std::forward<_Args>(__args)...);
-  __alloc_traits::construct(__a, std::__to_address(__v.__end_), std::forward<_Args>(__args)...);
-  __v.__end_++;
-  __swap_out_circular_buffer(__v);
-  return this->__end_;
-}
-
 template <class _Tp, class _Allocator>
 template <class... _Args>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
@@ -1551,16 +1543,14 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
     void
 #endif
     vector<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
-  pointer __end = this->__end_;
-  if (__end < this->__end_cap()) {
-    __construct_one_at_end(std::forward<_Args>(__args)...);
-    ++__end;
-  } else {
-    __end = __emplace_back_slow_path(std::forward<_Args>(__args)...);
-  }
-  this->__end_ = __end;
+      if (size() == capacity()) [[__unlikely__]] {
+        __reserve_for_real(__recommend(size() + 1));
+      }
+
+      // Now we're in the fast path, and we can assume that we have enough capacity.
+      __construct_one_at_end(std::forward<_Args>(__args)...);
 #if _LIBCPP_STD_VER >= 17
-  return *(__end - 1);
+      return *(this->__end_ - 1);
 #endif
 }

The other thing we investigated was to rewrite the code like this:

diff --git a/libcxx/include/vector b/libcxx/include/vector
index bfbf1ea1cfc9..3b62dc029b76 100644
--- a/libcxx/include/vector
+++ b/libcxx/include/vector
@@ -1542,6 +1542,32 @@ vector<_Tp, _Allocator>::__emplace_back_slow_path(_Args&&... __args) {
   return this->__end_;
 }
 
+// This function is basically doing the following code:
+//  if (cond) [[likely]]
+//    __if();
+//  else
+//    __else();
+//
+// However, it does so without preventing the optimizer from inlining
+// the `else` branch in the case where the condition is known at compile-time.
+// This should really be fixed in the optimizer instead, see <LLVM BUG>.
+// Once that bug is fixed, there is no reason to keep this utility around.
+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 _Allocator>
 template <class... _Args>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
@@ -1552,12 +1578,17 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
 #endif
     vector<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
   pointer __end = this->__end_;
-  if (__end < this->__end_cap()) {
-    __construct_one_at_end(std::forward<_Args>(__args)...);
-    ++__end;
-  } else {
-    __end = __emplace_back_slow_path(std::forward<_Args>(__args)...);
-  }
+
+  bool __has_capacity = __end < this->__end_cap();
+  std::__if_likely_else(__has_capacity,
+    [&] {
+        __construct_one_at_end(std::forward<_Args>(__args)...);
+        ++__end;
+    },
+    [&] {
+        __end = __emplace_back_slow_path(std::forward<_Args>(__args)...);
+    });
+
   this->__end_ = __end;
 #if _LIBCPP_STD_VER >= 17
   return *(__end - 1);

Basically, this applies [[likely]] on the no-reallocation branch, which makes the compiler outline the slow path like we want. However, we want the compiler to still constant-fold the capacity check when it can so as to completely fold the slow path when the vector shape is known. To make this happen, we need to work around what we view as an LLVM bug by dropping [[likely]]. We think this specific issue can be addressed, and once addressed, the weird __if_likely_else helper can go away.

[ldionne]

LGTM, but can you please run the SPEC benchmarks and provide the before/after comparison? I wouldn't be surprised if there was some sort of change.

[ldionne]

Suggested change

	// 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
	// This function performs an if-else on the given condition, calling either `__if()` or `__else()`. The `__if()` branch is annotated as being likely.
	//
	// However, it increases the likelihood that the `__else()` branch will be inlined if `__cond` is known to be a constant by
	// the optimizer, which LLVM currently doesn't do when naively applying the [[likely]] attribute.
	// See https://llvm.org/PR154292

[ldionne]

This is nice. https://godbolt.org/z/rK6EjTehG indeed shows that the codegen is a lot better.

[philnik777]

I've run the benchmarks and the difference seems to be within margin of error - possibly a tiny bit better.

[dzhidzhoev]

Please note that this patch has broken lldb-remote-linux-ubuntu.

======================================================================
FAIL: test_dwo (TestDbgInfoContentVectorFromStdModule.TestDbgInfoContentVector.test_dwo)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1828, in test_method
    return attrvalue(self)
           ^^^^^^^^^^^^^^^
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 155, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 155, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 155, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py", line 54, in test
    self.expect_expr("a.size()", result_type=size_type, result_value="3")
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2596, in expect_expr
    value_check.check_value(self, eval_result, str(eval_result))
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 301, in check_value
    test_base.assertSuccess(val.GetError())
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2631, in assertSuccess
    self.fail(self._formatMessage(msg, "'{}' is not success".format(error)))
AssertionError: 'error: Couldn't materialize: Invalid type: Cannot determine size
error: errored out in DoExecute, couldn't PrepareToExecuteJITExpression
' is not success

https://lab.llvm.org/buildbot/#/builders/195/builds/14502

[boomanaiden154]

This also broke premerge. We should probably get LLDB data formatters coverage here given it looks like the CI all passed here.

[boomanaiden154]

I've reverted this in 7f2e9b1 due to the lack of response here. Hopefully that doesn't cause too much trouble.

[Michael137]

I'll take a look at why the tests were affected by this next week

[nico]

For what it's worth, this patch regressed binary size on our chromium android binary size a bit.

With this patch: https://chromium-review.googlesource.com/c/chromium/src/+/6943035 ("APK Size (+50.9 KiB)")
With the revert: https://chromium-review.googlesource.com/c/chromium/src/+/6944345

It's a fairly small regression so it doesn't affect us much, but maybe worth mentioning.

The bot uses O2, ThinLTO, PGO, ICF, ffunction-sections / -fdata-sections.