Added 'inline' attribute to __init to inline the basic_string's constructor

[laxmansole]

Summary

basic_string's constructor calls init which was not getting inlined.
This prevented optimization of const string as init would appear as a call in between a string's def and use.

Worked in collaboration with Aditya Kumar.

[mclow.lists]

Do we have a test for the problem that this is solving?

[sebpop]

In D22782#495436, @mclow.lists wrote:

Do we have a test for the problem that this is solving?

I think we can write a testcase that shows that copy constructors are not optimized away unless the string constructor is inlined.

This patch fixes the performance of a proprietary benchmark when compiled with libc++, closing the performance gap with the same benchmark compiled with the gnu libstdc++. Overall with this patch we have half a billion fewer instructions out of about 10 billion.

[hiraditya]

In D22782#495436, @mclow.lists wrote:

Do we have a test for the problem that this is solving?

We are looking at the libcxx testsuite, it seems there are only correctness/functionality tests. Do you have any pointers on how to add tests to check if functions are inlined. In llvm it is easy to do because we can print the assembly and use FileCheck/grep to CHECK/CHECK-NOT if a string is present.

[laxmansole]

In D22782#495436, @mclow.lists wrote:

Do we have a test for the problem that this is solving?

$ cat foo.cpp

int foo(const std::string name);
int bar(){
    return foo("bar");
}

$clang++ -S -O3 -fno-exceptions foo.cpp

Assembly output without patch:

_Z3barv:                                // @_Z3barv
// BB#0:                                // %entry
    sub sp, sp, #64             // =64
    adrp    x1, .L.str
    add x1, x1, :lo12:.L.str
    add x0, sp, #8              // =8
    orr w2, wzr, #0x3
    stp xzr, x19, [sp, #24]     // 8-byte Folded Spill
    stp x29, x30, [sp, #48]     // 8-byte Folded Spill
    add x29, sp, #48            // =48
    stp xzr, xzr, [sp, #8]
    bl  _ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE6__initEPKcm
    add x0, sp, #8              // =8
    bl  _Z3fooNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE
    and w19, w0, #0x1
    add x0, sp, #8              // =8
    bl  _ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEED1Ev
    mov  w0, w19
    ldp x29, x30, [sp, #48]     // 8-byte Folded Reload
    ldr x19, [sp, #32]          // 8-byte Folded Reload
    add sp, sp, #64             // =64
    ret

Assembly output with patch:

_Z3barv:                                // @_Z3barv
// BB#0:                                // %entry
    sub sp, sp, #64             // =64
    orr w8, wzr, #0x6
    mov w9, #114
    mov w10, #24930
    add x0, sp, #8              // =8
    stp xzr, x19, [sp, #24]     // 8-byte Folded Spill
    stp x29, x30, [sp, #48]     // 8-byte Folded Spill
    add x29, sp, #48            // =48
    stp xzr, xzr, [sp, #8]
    strb    w8, [sp, #8]
    strb    w9, [sp, #11]
    sturh   w10, [sp, #9]
    strb    wzr, [sp, #12]
    bl  _Z3fooNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE
    and w19, w0, #0x1
    add x0, sp, #8              // =8
    bl  _ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEED1Ev
    mov  w0, w19
    ldp x29, x30, [sp, #48]     // 8-byte Folded Reload
    ldr x19, [sp, #32]          // 8-byte Folded Reload
    add sp, sp, #64             // =64
    ret

We can see that call to

_ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE6__initEPKcm

get inlined.

[laxmansole]

Ping

[EricWF]

The change itself LGTM, although we probably want to inline the forward/input iterator __init's as well.

However I would like to see a small benchmark that demonstrates the performance change. Please try and write the benchmark using Google Benchmark.
Some helpful links:

• http://libcxx.llvm.org/docs/TestingLibcxx.html#building-benchmarks
• http://github.com/google/benchmark

[hiraditya]

In D22782#504416, @EricWF wrote:

The change itself LGTM, although we probably want to inline the forward/input iterator __init's as well.

However I would like to see a small benchmark that demonstrates the performance change. Please try and write the benchmark using Google Benchmark.
Some helpful links:

• http://libcxx.llvm.org/docs/TestingLibcxx.html#building-benchmarks
• http://github.com/google/benchmark

Sure,
We'll come up with a synthetic benchmark to expose performance improvements.

Thanks,

[laxmansole]

Added inline attribute to the forward/input iterator __init's.
Thanks @EricWF for suggestion.

[EricWF]

I would love to see a benchmark with this, but I've done enough investigating on my own that I *know* this patch is beneficial.

[sebpop]

In D22782#512337, @EricWF wrote:

I would love to see a benchmark with this, but I've done enough investigating on my own that I *know* this patch is beneficial.

This patch was motivated by perf analysis we did on a proprietary benchmark in which we have seen a reduction of about 1 billion instructions (out of 10B) on x86_64-linux and aarch64-linux.

libcxx/trunk/include/string

 {
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__insert_c(this);
 #endif
     __zero();
 }
 
 template <class _CharT, class _Traits, class _Allocator>
+inline _LIBCPP_INLINE_VISIBILITY
 void
 basic_string<_CharT, _Traits, _Allocator>::__init(const value_type* __s, size_type __sz, size_type __reserve)
 {
     if (__reserve > max_size())
         this->__throw_length_error();
     pointer __p;
     if (__reserve < __min_cap)
     {
         __set_short_size(__sz);
         __p = __get_short_pointer();
     }
     else
     {
         size_type __cap = __recommend(__reserve);
         __p = __alloc_traits::allocate(__alloc(), __cap+1);
         __set_long_pointer(__p);
         __set_long_cap(__cap+1);
         __set_long_size(__sz);
     }
     traits_type::copy(_VSTD::__to_raw_pointer(__p), __s, __sz);
     traits_type::assign(__p[__sz], value_type());
 }
 
 template <class _CharT, class _Traits, class _Allocator>
+inline _LIBCPP_INLINE_VISIBILITY
 void
 basic_string<_CharT, _Traits, _Allocator>::__init(const value_type* __s, size_type __sz)
 {
     if (__sz > max_size())
         this->__throw_length_error();
     pointer __p;
     if (__sz < __min_cap)
     {
#if _LIBCPP_DEBUG_LEVEL >= 2
     if (__is_long())
         __get_db()->swap(this, &__str);
 #endif
 }
 
 #endif  // _LIBCPP_HAS_NO_RVALUE_REFERENCES
 
 template <class _CharT, class _Traits, class _Allocator>
+inline _LIBCPP_INLINE_VISIBILITY
 void
 basic_string<_CharT, _Traits, _Allocator>::__init(size_type __n, value_type __c)
 {
     if (__n > max_size())
         this->__throw_length_error();
     pointer __p;
     if (__n < __min_cap)
     {
basic_string<_CharT, _Traits, _Allocator>::basic_string(__self_view __sv, const allocator_type& __a)
     __init(__sv.data(), __sv.size());
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__insert_c(this);
 #endif
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 template <class _InputIterator>
+inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
     __is_exactly_input_iterator<_InputIterator>::value,
     void
 >::type
 basic_string<_CharT, _Traits, _Allocator>::__init(_InputIterator __first, _InputIterator __last)
 {
     __zero();
catch (...)
             __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
         throw;
     }
 #endif  // _LIBCPP_NO_EXCEPTIONS
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 template <class _ForwardIterator>
+inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
     __is_forward_iterator<_ForwardIterator>::value,
     void
 >::type
 basic_string<_CharT, _Traits, _Allocator>::__init(_ForwardIterator __first, _ForwardIterator __last)
 {
     size_type __sz = static_cast<size_type>(_VSTD::distance(__first, __last));