[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.

[Devin Jeanpierre via Phabricator via libcxx-commits]

devin.jeanpierre added a comment.

Right now I'm looking at running a macrobenchmark internally to Google to test this change -- the first one I ran (late last year) gave only noise as a result, but I've been pointed at a better macrobenchmark that might give more reliable and useful numbers.

The idea being that this could fill out the big "TODO" in the PR description.



================
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) > {};
----------------
Quuxplusone wrote:
> 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.
I renamed to `__has_default_allocator_construct`.

@Quuxplusone that's an good idea! Actually, since C++20 deletes construct from the standard allocator, that effectively means it's "just" backporting optimizations to C++17 and below, I think.


================
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,
----------------
devin.jeanpierre wrote:
> Quuxplusone wrote:
> > 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.)
> Yeah, this surprised me too. But in fact, this pattern is already used elsewhere, e.g. `__split_buffer`: https://github.com/llvm-mirror/libcxx/blob/master/include/__split_buffer#L296-L312
> 
>  I asked around and was told it's normal-looking code, and it was in the original revision, so I kept it as is. (I still need to add some kind of comment, as was requested in the original D67524.)
> 
> A problem is that if you're moving an element, then you only move via memcpy if it's trivially movable. If you're relocating via memcpy, then you additionally move via memcpy when it's trivially relocatable. And then, on the destruction side, you only destroy if it was a relocation operation, and was moved via memcpy.
> 
> So there are three options that I can think of:
> 
> 1. keep the code as it was before with the enable_if. Callers must, to do a trivial relocation, first reinterpret_cast their pointers to char so that it's picked up by the trivial enable_if. (This is what I would've done if I weren't copying an existing PR, but I'm not sure if we're allowed to use constexpr if inside vector. Also, that both `__split_buffer` and the pre-existing PR use the true_type/etc. trick, I'm worried this would be unusual.)
> 2. keep the code as it was before with the enable_if, but also add a check for trivial relocatability. If the type is trivially relocatable, callers are obligated to release the underlying storage without invoking the destructor. Otherwise, this is a normal move operation. (This, then, becomes a relocation function, and callers must uphold relocation postconditions.)
> 3. this version of the code: have an explicit selector for trivial vs nontrivial operations.
> 
> My biggest issue with #2 is that it hardcodes that this is relocation -- if someone wanted to move and keep the moved-from values, rather than relocate, we'd need to write a second function. I think in this case it's only used for relocation, so it's fine.
> 
> Do you have a preference among the 3 options?
Kept as is, and added a comment.


================
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
----------------
Quuxplusone wrote:
> devin.jeanpierre wrote:
> > 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.
> > Are you requesting I delete `__libcpp_is_trivially_relocatable` and `__libcpp_is_trivially_relocatable_v`, or just remove them from the file comment?
> > 
> > I'm fine deleting even the symbols themselves, but wouldn't know how best to share code in that case, as both vector and e.g. swap could want to use this.
> > 
> > (That said, I do at some point want to offer this up as an optimization opportunity for absl. Of course, absl would just do the same __has_keyword dance, regardless, as they can't depend specifically on libc++. I think that might be the portability trap you mention. :) )
> 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?
Removed from the synopsis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385