[libcxx-commits] [PATCH] D112017: [libcxx][format] Fix how we handle char traits in formatter<string> and formatter<string_view>

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__format/formatter_string.h:147
+
+  _LIBCPP_HIDE_FROM_ABI auto format(const basic_string_view<_CharT, _Traits>& __str, auto& __ctx)
+      -> decltype(__ctx.out()) {
----------------
DanielMcIntosh-IBM wrote:
> DanielMcIntosh-IBM wrote:
> > Quuxplusone wrote:
> > > Style: Linebreak after `auto`, please.
> > > 
> > > Perf: Shouldn't `basic_string_view<_CharT, _Traits> __str` be passed by value, not by const-ref? Or is there a subtle reason we're passing by const-ref here in particular?
> > I believe `formatter<basic_string_view<_CharT, _Traits>, _CharT>::format` needs to meet the [Formatter named requirements](https://en.cppreference.com/w/cpp/named_req/Formatter), which limits what the function signature can be.
> Never mind, I realized passing by value won't change what we can accept as arguments.
> 
> However, I'm not quite sure why passing by value would be a performance benefit. When `_Traits == char_traits<_CharT>`, does the `basic_string_view<_CharT>(__str.data(), __str.size())` somehow get treated as a move constructor, allowing the re-use of `__str` as the argument to `_Base::format` if `__str` was passed by value? i.e. does using that specific constructor somehow still allow copy elision? I was under the impression it was only allowed if "the initializer expression is a [value] of the same class type". Or is this a case of "we know what the constructor does, so the optimizer can figure out that there are no side-effects, and remove it"?
Pass-by-value has basically three perf benefits, in the abstract:
- Eliminate a pointer indirection in the callee, which means eliminate a load from memory. `string_view` can be passed in registers... but if you pass it by reference, it spills into memory. https://godbolt.org/z/zqr6TjEoG
- Eliminate the need to spill the variable onto the stack in the caller, which means maybe eliminate the entire need for a stack frame in the caller. https://godbolt.org/z/G1Kj65n15
- Give the callee its own copy of the `string_view` object, which means no aliasing is possible with anything else in the program, which means greater opportunities for optimization. https://godbolt.org/z/zddj8Yn4T

For `string_view` (and `int` and `char*` and other small trivial types), the rule is "pass by value"; and then for big or expensive types (like `string` or `vector<int>`) the rule becomes "pass by const reference, as an optimization of pass by value." But only because avoiding-that-one-copy is big enough to outweigh all three of the benefits above! (And because for non-trivially-copyable types such as `string`, pass-by-value secretly turns into pass-by-hidden-reference on most ABIs we care about. So really there's not even a competition in those non-trivial cases.)

Now, inliners are really good, and it's //quite probable// that passing by const reference in this specific case is harmless in practice. But since pass-`string_view`-by-value is the Right Thing To Do, here I would want to see a //positive reason to deviate// from the best practice. I'd rather keep our code as idiomatic as possible, so that when someone (me ;)) looks at it, they go "oh yeah, cool, that's what I expect to see" instead of "wait, is something subtle going on here? why does this look weird?" So "pass small types including `string_view` by value" is the default.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112017/new/

https://reviews.llvm.org/D112017