Partially inline basic_string copy constructor in UNSTABLE

[mvels]

Summary

This change splits the copy constructor up inlining short initialization, and explicitly outlining long initialization into __init_copy_ctor_external() which is the externally instantiated slow path.

For unstable ABI, this has the following changes:

• remove basic_string(const basic_string&)
• remove basic_string(const basic_string&, const Allocator&)
• add __init_copy_ctor_external(const value_type*, size_type)

Quick local benchmark for Copy:

Master

---------------------------------------------------------------
Benchmark                    Time             CPU   Iterations
---------------------------------------------------------------
BM_StringCopy_Empty       3.50 ns         3.51 ns    199326720
BM_StringCopy_Small       3.50 ns         3.51 ns    199510016
BM_StringCopy_Large       15.7 ns         15.7 ns     45230080
BM_StringCopy_Huge        1503 ns         1503 ns       464896

With this change

---------------------------------------------------------------
Benchmark                    Time             CPU   Iterations
---------------------------------------------------------------
BM_StringCopy_Empty       1.99 ns         2.00 ns    356471808
BM_StringCopy_Small       3.29 ns         3.30 ns    203425792
BM_StringCopy_Large       13.3 ns         13.3 ns     52948992
BM_StringCopy_Huge        1472 ns         1472 ns       475136

[EricWF]

libcxx/test/std/strings/basic.string/string.cons/copy_shrunk_long.pass.cpp

3

This file needs to go under test/libcxx/... instead of test/std/.

The former directory is for libc++ specific tests, the latter is for tests that
should pass with all standard library implementations.

[EricWF]

This patch should contain the changes to the unstable extern template list.

Also, the added test should pass both before and after your change, correct?
That is: does this change introduce a behavioral change, or is it simply an optimization?

Otherwise, this patch LGTM.

libcxx/include/string

1553

This isn't always externally instantiated.
And it doesn't assert that __s is null terminated (nor can it)

What's this comment really trying to say?

1907

It's a bit unfortunate that this function essentially duplicates __init(char*, size_type).

Can you write a short comment as to why this provides benefit over simply calling __init?
The formulation is essentially the same.

[mvels]

Update comments

[mvels]

Updated comments, synced with master

[mvels]

Diff vs origin

[mvels]

• Moved test

[EricWF]

LGTM.

[mvels]

Rebased

[mvels]

Thanks, arc is not my friend :\ Fixed the diff using --base=git:origin

[mvels]

BTW, you could argue 'arc' was smart enough to recognize this was a rolled forward revert, and only diff against the original submit, alas :)

libcxx/include/__string

#define _LIBCPP_STRING_V1_EXTERN_TEMPLATE_LIST(_Func, _CharType) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::iterator basic_string<_CharType>::insert(basic_string::const_iterator, value_type)) \
   _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::resize(size_type, value_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::insert(size_type, basic_string const&, size_type, size_type))
 
 #define _LIBCPP_STRING_UNSTABLE_EXTERN_TEMPLATE_LIST(_Func, _CharType) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::replace(size_type, size_type, value_type const*, size_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::rfind(value_type const*, size_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__init(value_type const*, size_type, size_type)) \
-  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::basic_string(basic_string const&)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::replace(size_type, size_type, value_type const*)) \
-  _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::basic_string(basic_string const&, std::allocator<_CharType> const&)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find_last_not_of(value_type const*, size_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::~basic_string()) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find_first_not_of(value_type const*, size_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::insert(size_type, size_type, value_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::operator=(value_type)) \
   _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__init(value_type const*, size_type)) \
+  _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::__init_copy_ctor_external(value_type const*, size_type)) \
   _Func(_LIBCPP_FUNC_VIS const _CharType& basic_string<_CharType>::at(size_type) const) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::insert(size_type, value_type const*, size_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>::size_type basic_string<_CharType>::find_first_of(value_type const*, size_type, size_type) const) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::replace(size_type, size_type, size_type, value_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::assign(value_type const*, size_type)) \
   _Func(_LIBCPP_FUNC_VIS void basic_string<_CharType>::reserve(size_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::append(value_type const*, size_type)) \
   _Func(_LIBCPP_FUNC_VIS basic_string<_CharType>& basic_string<_CharType>::assign(basic_string const&, size_type, size_type)) \

libcxx/include/string

#endif  // _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
 
     inline
     void __init(const value_type* __s, size_type __sz, size_type __reserve);
     inline
     void __init(const value_type* __s, size_type __sz);
     inline
     void __init(size_type __n, value_type __c);
 
+    // Slow path for the (inlined) copy constructor for 'long' strings.
+    // Always externally instantiated and not inlined.
+    // Requires that __s is zero terminated.
+    // The main reason for this function to exist is because for unstable, we
+    // want to allow inlining of the copy constructor. However, we don't want
+    // to call the __init() functions as those are marked as inline which may
+    // result in over-aggressive inlining by the compiler, where our aim is
+    // to only inline the fast path code directly in the ctor.
+    void __init_copy_ctor_external(const value_type* __s, size_type __sz);
+
     template <class _InputIterator>
     inline
     _EnableIf
     <
         __is_exactly_cpp17_input_iterator<_InputIterator>::value
     >
     __init(_InputIterator __first, _InputIterator __last);
 

 
 template <class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>::basic_string(const basic_string& __str)
     : __r_(__default_init_tag(), __alloc_traits::select_on_container_copy_construction(__str.__alloc()))
 {
     if (!__str.__is_long())
         __r_.first().__r = __str.__r_.first().__r;
     else
-        __init(_VSTD::__to_address(__str.__get_long_pointer()), __str.__get_long_size());
+        __init_copy_ctor_external(_VSTD::__to_address(__str.__get_long_pointer()),
+                                  __str.__get_long_size());
+
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__insert_c(this);
 #endif
 }
 
 template <class _CharT, class _Traits, class _Allocator>
 basic_string<_CharT, _Traits, _Allocator>::basic_string(
     const basic_string& __str, const allocator_type& __a)
     : __r_(__default_init_tag(), __a)
 {
     if (!__str.__is_long())
         __r_.first().__r = __str.__r_.first().__r;
     else
-        __init(_VSTD::__to_address(__str.__get_long_pointer()), __str.__get_long_size());
+        __init_copy_ctor_external(_VSTD::__to_address(__str.__get_long_pointer()),
+                                  __str.__get_long_size());
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__insert_c(this);
 #endif
 }
 
 #ifndef _LIBCPP_CXX03_LANG
 
 template <class _CharT, class _Traits, class _Allocator>

 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__insert_c(this);
     if (__is_long())
         __get_db()->swap(this, &__str);
 #endif
 }
 
 template <class _CharT, class _Traits, class _Allocator>
+void basic_string<_CharT, _Traits, _Allocator>::__init_copy_ctor_external(
+    const value_type* __s, size_type __sz) {
+  pointer __p;
+  if (__sz < __min_cap) {
+    __p = __get_short_pointer();
+    __set_short_size(__sz);
+  } else {
+    if (__sz > max_size())
+      this->__throw_length_error();
+    size_t __cap = __recommend(__sz);
+    __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_address(__p), __s, __sz + 1);
+}
+
+template <class _CharT, class _Traits, class _Allocator>
 inline
 basic_string<_CharT, _Traits, _Allocator>::basic_string(basic_string&& __str, const allocator_type& __a)
     : __r_(__default_init_tag(), __a)
 {
     if (__str.__is_long() && __a != __str.__alloc()) // copy, not move
         __init(_VSTD::__to_address(__str.__get_long_pointer()), __str.__get_long_size());
     else
     {

libcxx/test/std/strings/basic.string/string.cons/copy_shrunk_long.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <string>
+
+// basic_string(const basic_string<charT,traits,Allocator>& str);
+
+#include <string>
+#include <cassert>
+
+#include "test_macros.h"
+#include "test_allocator.h"
+#include "min_allocator.h"
+
+template <class S>
+void
+test()
+{
+  // Tests that a long string holding a SSO size string results in
+  // an SSO copy constructed value.
+  S s1("1234567890123456789012345678901234567890123456789012345678901234567890");
+  s1.resize(7);
+  S s2(s1);
+  LIBCPP_ASSERT(s2.__invariants());
+  assert(s2 == s1);
+  assert(s2.capacity() < sizeof(S));
+}
+
+int main(int, char**)
+{
+    {
+    typedef test_allocator<char> A;
+    typedef std::basic_string<char, std::char_traits<char>, A> S;
+    test<S>();
+    }
+#if TEST_STD_VER >= 11
+    {
+    typedef min_allocator<char> A;
+    typedef std::basic_string<char, std::char_traits<char>, A> S;
+    test<S>();
+    }
+#endif
+
+  return 0;
+}