[libc++] Optimize string operator[] for known large inputs

[TocarIP]

If we know that index is larger than SSO size, we know that we can't be in SSO case, and should access the pointer. This removes extra check from operator[] for inputs known at compile time to be larger than SSO.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Ilya Tocar (TocarIP)

Changes

If we know that index is larger than SSO size, we know that we can't be in SSO case, and should access the pointer. This removes extra check from operator[] for inputs known at compile time to be larger than SSO.

---

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

1 Files Affected:

• (modified) libcxx/include/string (+6)

diff --git a/libcxx/include/string b/libcxx/include/string
index 91935162f02383a..cf9f0c847eb43af 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1198,11 +1198,17 @@ public:
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_reference operator[](size_type __pos) const _NOEXCEPT {
     _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__pos <= size(), "string index out of bounds");
+    if (__builtin_constant_p(__pos) && !__fits_in_sso(__pos)) {
+      return *(__get_long_pointer() + __pos);
+    }
     return *(data() + __pos);
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 reference operator[](size_type __pos) _NOEXCEPT {
     _LIBCPP_ASSERT_VALID_ELEMENT_ACCESS(__pos <= size(), "string index out of bounds");
+    if (__builtin_constant_p(__pos) && !__fits_in_sso(__pos)) {
+      return *(__get_long_pointer() + __pos);
+    }
     return *(__get_pointer() + __pos);
   }
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[EricWF]

Could you state how this optimization was measured?

[TocarIP]

I've looked at disassembly and verified that __is_long branch disappeared.

[ldionne]

This LGTM. I tried finding a downside and I couldn't. In general I'm not a huge fan of adding complexity like this, but in this case we're basically leveraging information known at compile-time (when it is known) to generate better code, and we do that in one of the most important types in the library. So I feel like anything reasonable that improves performance here is welcome.

In particular, I suspect this might make it easier for the compiler to vectorize code in case we have multiple constant accesses side by side because we'd remove branches, but TBH I don't know. At least this optimization can't generate worst code, so that's really comforting.

@EricWF I'll wait until like the middle of next week before merging this to give you time to react in case you had any concerns (as suggested by your question).

[EricWF]

I would like to see beenchmark results for this.

[TocarIP]

What benchmark do you want me to run?

[EricWF]

I would like you to write benchmarks that target the function you modified, ensure that you're case is covered there, then run them to show a performance improvement, as well as to verify that no regression occurred.

[TocarIP]

For the following benchmark

std::string s = "12345678901234567890123456789012345678901234567890";

void BM_index(benchmark::State& state) {
  for (auto _ : state) {
    testing::DoNotOptimize(s[33]*37+s[65]);
  }
}

Performance goes from

name      cpu/op 
BM_index  0.69ns ± 1%

To

name      cpu/op 
BM_index  0.46ns ± 1%

For a ~30% speed-up

[EricWF]

And what about for non-constant string indexes? Or what happens in a benchmark where you mix constant and non-constant indexes?

I would like this to be a little more thorough

[TocarIP]

For mixed index and following code

  void BM_index(benchmark::State& state) {
    for (auto _ : state) {
      for (int i = 0;i<21;i++) {
        testing::DoNotOptimize(s[33]*37+s[i]);
      }   
    }
  }

I see speed up from BM_index 15.5ns ±15% to BM_index 13.6ns ±16%
Non-constant index are obviously unchanged (16.0ns ±11% vs BM_index 16.3ns ±10%).
Overall its hard to write benchmarks when I don't have a good mental model for a possible regression. Do you have a specific concern in mind? Only code path with constant index is affected and removing branches seems like an obviously good thing (modulo code-alignment shenanigans).

[ldionne]

And what about for non-constant string indexes?

Right, in this case __builtin_constant_p will evaluate to false, so it is extremely unlikely that the compiler will generate any code for the if branch. Basically this seems to be a free optimization.

[ldionne]

I just spoke with @EricWF who said he was fine with this change. I'll merge it now. Thanks everyone for chiming in!