[libcxx-commits] [PATCH] D119385: Use trivial relocation operations in std::vector, by porting D67524 and part of D61761 to work on top of the changes in D114732.

Thu Feb 10 10:31:30 PST 2022

Quuxplusone added inline comments.

================
Comment at: libcxx/include/__memory/allocator_traits.h:1
 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
----------------
Pulling this out into somewhere we can thread the discussion:

> Also, I'd like to understand the relationship between this patch and Arthur's series of patches for trivially relocatable types -- is the intent that this one replaces the series (even though it's a slimmer version of Arthur's patches IIUC)?

As of D114732 / D119017, Clang now supports a built-in type trait `__is_trivially_relocatable(T)` which is currently `true` for types that are //trivial for purposes of ABI// and `false` for everyone else (including e.g. `std::string` and `std::unique_ptr`-with-the-default-ABI). The intent is that this state of affairs is not permanent, and eventually we'd like to extend the built-in type trait `__is_trivially_relocatable(T)` to encompass a greater set of types, perhaps corresponding to p1144, but we're not //committing// to exactly p1144 right now. But the people in charge of the `[[clang::trivial_abi]]` attribute are comfortable promising that //those// types will //always// be trivially relocatable in every possible sense of the word.

Clang //does not// currently support any way to "opt in" arbitrary types to trivial relocatability. For now, the only way to "become" trivially relocatable is to become also `[[clang::trivial_abi]]` at the same time.

This PR adds optimizations to `std::vector<T>` when `__is_trivially_relocatable<T>` (i.e., part of D67524), and also adds enough boilerplate in `<type_traits>` to support that optimization (i.e., part of D61761). It does //not// "opt in" any STL types — e.g., `std::vector<int>` itself will not be considered trivially relocatable even after this PR — because, as I said above, that's physically impossible in Clang so far.

This PR, if adopted, //would// shrink the diff between my `trivially-relocatable` branch and `main`, i.e., it's "friendly" in that sense.

I'm not saying it's a good idea, but I'm definitely not saying it's a bad idea. :)

================
Comment at: libcxx/include/__memory/allocator_traits.h:401
+template <class _Alloc, class _Tp, class... _Args>
+struct __has_trivial_construct : integral_constant<bool, (__is_default_allocator<_Alloc>::value ||
+                                                          !__has_construct<_Alloc, _Tp*, _Args...>::value) > {};
----------------
ldionne wrote:
> I find this misleading. Indeed, the construction of `_Tp` might not be trivial at all, it's just that the allocator doesn't do something fancy in addition to constructing the object. In other words, I think something like `__allocator_construct_is_placement_new` is what we *mean*, but that name sucks. Other potential names: `__has_default_allocator_construct`, `__overrides_allocator_construct`, `__has_custom_allocator_construct`.
> 
> In particular, I would avoid mentioning "trivial" anywhere because it makes it look as though the operation itself is trivial, which is not what we mean.
> 
> The same comment applies to `__has_trivial_destroy` below.
The "construct" and "destroy" operations //of the allocator// are "trivial" in the sense that they run no (additional) code — kind of like a trivial default constructor or trivial destructor runs no code, except that ctor/dtor is a leaf on the call graph, whereas `allocator::construct` and `allocator::destroy` are non-leaves.

However, trunk already has traits named `__has_construct` and `__has_destroy` — and I think they could profitably be modified to just include the check for `__is_default_allocator` themselves, couldn't they? That should be its own PR, but if it works it'd be cool. It would basically mean that `allocator_traits<allocator<T>>::construct` would skip a level of indirection but //should// have exactly the same behavior.

================
Comment at: libcxx/include/memory:852
 _LIBCPP_INLINE_VISIBILITY
-void __construct_forward_with_exception_guarantees(_Alloc& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2) {
+void __construct_forward_with_exception_guarantees(_Alloc& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2, false_type) {
     static_assert(__is_cpp17_move_insertable<_Alloc>::value,
----------------
ldionne wrote:
> I find those out-of-the-blue `false_type` and `true_type`s awkward. Instead, I would much rather make the decision of which overload to take based on `enable_if` right here, in these functions. That way, `std::vector` would just call `__construct_forward_with_exception_guarantees(a, b, e, b2)` without having to pass `__move_via_memcpy()`.
@ldionne, you and I had this conversation 2 or 3 years ago, and I'm still of the same opinion: the condition is complicated and it's really really important that the //caller// understand exactly what the condition is, so that they can pass the matching type to both callees. See lines 923–939 in `<vector>`. I //really// think this way is clearer.
(That said, I agree we might think about ways to comment `false_type` with something like "don't use memcpy" and `true_type` with something like "just use memcpy" — I have no great suggestion for the wording or where it'd go exactly.)

================
Comment at: libcxx/include/type_traits:357-358
         = is_trivially_destructible<T>::value;                           // C++17
+      template <class T> inline constexpr bool __libcpp_is_trivially_relocatable_v
+        = __libcpp_is_trivially_relocatable<T>::value;                   // extension
       template <class T, class... Args> inline constexpr bool is_nothrow_constructible_v
----------------
ldionne wrote:
> I'm fine if we make optimizations inside the library, but I don't want us to expose a new "customization point" that hasn't been standardized. That's just going to be a pain in the future, and it's a portability trap for our users.
Just like all type traits, it is UB for the user to specialize or otherwise customize this type-trait.
You can't say
```
template<> struct std::is_trivially_move_constructible<Widget> : std::true_type {};
```
The `__libcpp_` should be good enough to warn people away from querying the trait willy-nilly.
Or are you just asking to remove this "documentation" from the synopsis?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385