[ADT] Pass string_view via copy in twine

[prehistoric-penguin]

Summary

Take the cheap object std::string_view by copy is the preferred way:

They have performance differences, pass by reference has one more level
of indirection, so we need one more instruction when compared with the
copy way For such code snippet(https://godbolt.org/z/19rWfd96W):

void TakesStringViewByValue(std::string_view s)
{
benchmark::DoNotOptimize(s.data());
}
void TakesStringViewByRef(const std::string_view& s)
{
benchmark::DoNotOptimize(s.data());
}

When getting the disassembly after build with -O3, pass by value will save us
one instruction:

TakesStringViewByValue(std::basic_string_view<char, std::char_traits<char> >):
movq %rsi, -8(%rsp)
retq
TakesStringViewByRef(std::basic_string_view<char, std::char_traits<char> > const&):
movq 8(%rdi), %rax
movq %rax, -8(%rsp)
retq
_GLOBAL__sub_I_example.cpp: # @_GLOBAL__sub_I_example.cpp
jmp benchmark::internal::InitializeStreams()@PLT # TAILCALL

And according to the abseil c++ tips week 1(https://abseil.io/tips/1),
they also recommend us to pass std::string_view by copy:

Unlike other string types, you should pass string_view by value just
like you would an int or a double because string_view is a small value.

[prehistoric-penguin]

Could you please review this minor change?

[saugustine]

Why should these be different than the other constructors which don't take copies?

[prehistoric-penguin]

In D129533#3644282, @saugustine wrote:

Why should these be different than the other constructors which don't take copies?

Thanks, take the cheap object std::string_view by copy is the preferred way:

• They have performance differences, pass by reference has one more level of indirection, so we need one more instruction when compared with the copy way

For such code snippet:

#include <string>
#include <benchmark/benchmark.h>
void TakesStringViewByValue(std::string_view s)
{
    benchmark::DoNotOptimize(s.data());
}
void TakesStringViewByRef(const std::string_view& s)
{
    benchmark::DoNotOptimize(s.data());
}

We get the disassembly after build with -O3:

TakesStringViewByValue(std::basic_string_view<char, std::char_traits<char> >): # @TakesStringViewByValue(std::basic_string_view<char, std::char_traits<char> >)
  movq %rsi, -8(%rsp)
  retq
TakesStringViewByRef(std::basic_string_view<char, std::char_traits<char> > const&): # @TakesStringViewByRef(std::basic_string_view<char, std::char_traits<char> > const&)
  movq 8(%rdi), %rax
  movq %rax, -8(%rsp)
  retq
_GLOBAL__sub_I_example.cpp: # @_GLOBAL__sub_I_example.cpp
  jmp benchmark::internal::InitializeStreams()@PLT # TAILCALL

And according to the abseil c++ tips week 1, they also recommend us to pass std::string_view by copy:

Unlike other string types, you should pass string_view by value just like you would an int or a double because string_view is a small value.

[RKSimon]

Remind me - can we forward declare string_view or is it a bulky header dependent nightmare like std::string ?

llvm/include/llvm/ADT/Twine.h

304

This probably should be done seperately and part of a larger refactor away from const StringRef & args IF we can confirm that its beneficial - it will probably need an update in the coding guidelines as well.

[prehistoric-penguin]

Fix concern

[prehistoric-penguin]

llvm/include/llvm/ADT/Twine.h

304

Thanks! Could you please elaborate more on larger refactor? I am interested in this.

[RKSimon]

llvm/include/llvm/ADT/Twine.h

304

We have hundreds of uses of "const StringRef &" as function arguments - and the trade off will be to determine whether we get improvement in performance vs the additional cost of having to define StringRef in more headers.

<string> is already one of the most costly headers to include in llvm and making that worse (via including StringRef.h) doesn't sounds like a good idea: https://commondatastorage.googleapis.com/chromium-browser-clang/llvm-include-analysis.html

[RKSimon]

llvm/include/llvm/ADT/Twine.h

293–294

Please can you add to the comment that passing string_view by value is preferred.

[prehistoric-penguin]

[ADT] Pass string_view via copy in twine

[prehistoric-penguin]

This comment was removed by prehistoric-penguin.

llvm/include/llvm/ADT/Twine.h

304

Thanks for the comment, I'm going to do some experiments and update the results of my research later.

[RKSimon]

The string_view only patch looks almost ready and can be handled separately from any StringRef changes you make - please can you add a better subject description next?

[prehistoric-penguin]

In D129533#3665132, @RKSimon wrote:

The string_view only patch looks almost ready and can be handled separately from any StringRef changes you make - please can you add a better subject description next?

Thanks! Could you please explain better subject description next? I guess I need to change the title here. Or to make a better description in the features patcher?

[RKSimon]

Explain why the patch passes the string_view via copy

[prehistoric-penguin]

Update commit message

[RKSimon]

You're still missing a patch description explaining why pass by value is preferred

[prehistoric-penguin]

Take the cheap object std::string_view by copy is the preferred way:

They have performance differences, pass by reference has one more level
of indirection, so we need one more instruction when compared with the
copy way For such code snippet(https://godbolt.org/z/19rWfd96W):

void TakesStringViewByValue(std::string_view s)
{
benchmark::DoNotOptimize(s.data());
}
void TakesStringViewByRef(const std::string_view& s)
{
benchmark::DoNotOptimize(s.data());
}

When get the disassembly after build with -O3, pass by value will save us
one instruction:

TakesStringViewByValue(std::basic_string_view<char, std::char_traits<char> >):
movq %rsi, -8(%rsp)
retq
TakesStringViewByRef(std::basic_string_view<char, std::char_traits<char> > const&):
movq 8(%rdi), %rax
movq %rax, -8(%rsp)
retq
_GLOBAL__sub_I_example.cpp: # @_GLOBAL__sub_I_example.cpp
jmp benchmark::internal::InitializeStreams()@PLT # TAILCALL

And according to the abseil c++ tips week 1(https://abseil.io/tips/1),
they also recommend us to pass std::string_view by copy:

 Unlike other string types, you should pass string_view by value just
like you would an int or a double because string_view is a small value.

[prehistoric-penguin]

In D129533#3668063, @RKSimon wrote:

You're still missing a patch description explaining why pass by value is preferred

Thanks, finally I realize maybe you're referring to the summary part!

[RKSimon]

LGTM cheers

llvm/include/llvm/ADT/Twine.h

public:
     /*implicit*/ Twine(const std::string &Str) : LHSKind(StdStringKind) {
       LHS.stdString = &Str;
       assert(isValid() && "Invalid twine!");
     }
 
 #if __cplusplus > 201402L
     /// Construct from an std::string_view by converting it to a pointer and
     /// length.  This handles string_views on a pure API basis, and avoids
     /// storing one (or a pointer to one) inside a Twine, which avoids problems
-    /// when mixing code compiled under various C++ standards.
-    /*implicit*/ Twine(const std::string_view &Str)
-        : LHSKind(PtrAndLengthKind) {
+    /// when mixing code compiled under various C++ standards. Pass string_view
+    /// by value is preferred.
+    /*implicit*/ Twine(std::string_view Str) : LHSKind(PtrAndLengthKind) {
       LHS.ptrAndLength.ptr = Str.data();
       LHS.ptrAndLength.length = Str.length();
       assert(isValid() && "Invalid twine!");
     }
 #endif
 
     /// Construct from a StringRef.
     /*implicit*/ Twine(const StringRef &Str) : LHSKind(PtrAndLengthKind) {
       LHS.ptrAndLength.ptr = Str.data();
       LHS.ptrAndLength.length = Str.size();
       assert(isValid() && "Invalid twine!");
     }
 
     /// Construct from a SmallString.
     /*implicit*/ Twine(const SmallVectorImpl<char> &Str)
         : LHSKind(PtrAndLengthKind) {