[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 12:01:37 PST 2022

devin.jeanpierre marked 2 inline comments as done.
devin.jeanpierre added a comment.

No changes, just some quick replies and requests for clarification at the start of my day. :)

Thanks for the comments! If I didn't reply to one here, it's probably because I agree with it and will implement the change. :D

---

In D119385#3309616 <https://reviews.llvm.org/D119385#3309616>, @Quuxplusone wrote:

> - I think you're selling `std::swap` short; it's used by `rotate` and `sort` and all kinds of cool algorithms that would be nice to speed up. We should figure out how to get `std::swap` to bitwise-swap trivially relocatable things, either by solving the problems or by decreeing that they shall not matter. :)

We certainly can do std::swap correctly, but it's annoying! Doing it optimally would probably best be done via an additional compiler intrinsic to tell us the dsize of a type -- we can probably do without that intrinsic with a recursive templated function, but it's going to slow down compiles and be otherwise difficult.

The easiest way to make forward progress on `std::swap` is to start with `std::has_unique_object_representations`. That is overbroad but guarantees correctness AFAIK.

> - Might make sense to split out the addition of `__libcpp_is_trivially_relocatable` into its own PR, and then add the vector optimization as a child PR of that one, as I did with the D50119 <https://reviews.llvm.org/D50119> patch series. However, it could also be argued that you shouldn't add `__libcpp_is_trivially_relocatable` without any consumers of it. So I'm ambivalent, and would wait to see what others think. (Similarly, there's the question of how to land it: one commit or two? If we end up having to revert it unexpectedly, should we have the option to revert just the vector optimization, or should we force ourselves to revert //all// of it to avoid leaving `__libcpp_is_trivially_relocatable` as a loose end with no consumers?)

These should *probably* be two commits, but I am still new to phabricator and didn't know how to do it. 👀

FWIW, I don't anticipate any users actually using `__libcpp_is_trivially_relocatable`, so it isn't very important to submit or roll back separately other than for convenience of review. (Anyone that wanted to use it would instead redefine it, so that their code works on other C++ standard library implementations.)

In D119385#3309652 <https://reviews.llvm.org/D119385#3309652>, @Quuxplusone wrote:

> @devin.jeanpierre re `__is_default_allocator`: You should take a look at https://github.com/Quuxplusone/llvm-project/compare/main...trivially-relocatable and specifically https://github.com/Quuxplusone/llvm-project/commit/dfbfc8b326e0f5a3bcc284e3a28893310b30d0bb , because that's the current source of truth for https://p1144.godbolt.org/ . That already includes some stuff like the memmove bugfix and the switch to `__is_default_allocator`.

Thanks for the link! I'm going to try to do a diff and see if that pans out, or check if I need to look at it line by line :P

================
Comment at: libcxx/include/__memory/allocator_traits.h:1
 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
----------------
Quuxplusone wrote:
> 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. :)
+1 to this summary.

================
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,
----------------
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?

================
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.
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. :) )

================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.pass.cpp:90
+  static_assert(std::__libcpp_is_trivially_relocatable<TrivialMoveOnly>::value, "");
+  static_assert(!std::__libcpp_is_trivially_relocatable<const TrivialMoveOnly>::value, "");
+#endif
----------------
Quuxplusone wrote:
> I would say that in a perfect world `const TrivialMoveOnly` ought to be trivially relocatable, because `TrivialMoveOnly` is a trivially relocatable type, and if you're moving-and-destroying it, then you don't really care that the source object was const-qualified.
> However, I see that even under p1144r5, `is_relocatable_v<const TrivialMoveOnly>` would be `false`, and I did not intend that `is_trivially_relocatable_v<T> && !is_relocatable_v<T>` should ever be true for any `T`. So I'll have to change this part of p1144. Most library users, then, should probably use `is_trivially_relocatable<remove_cvref_t<T>>` for their purposes.
> 
> Relevant question for this PR: Should we permit the libc++ extension type `std::vector<const X>` to actually `push_back` and reallocate when `is_trivially_relocatable_v<X>`? In the past that was obviously impossible, but now it's kinda possible.
> Relevant question for this PR: Should we permit the libc++ extension type `std::vector<const X>` to actually `push_back` and reallocate when `is_trivially_relocatable_v<X>`? In the past that was obviously impossible, but now it's kinda possible.

My personal opinion is that this is too dangerous: people will rely on it by accident, not even knowing what they're doing (e.g. templated code with levels of indirection), and that would make it significantly harder to remove or change `__is_trivially_relocatable`.

Separately (bit of a cop-out), I think that should be a separate PR: it's a new feature, whereas this PR is "just" an optimization, if that makes sense.

================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.pass.cpp:90
+  static_assert(std::__libcpp_is_trivially_relocatable<TrivialMoveOnly>::value, "");
+  static_assert(!std::__libcpp_is_trivially_relocatable<const TrivialMoveOnly>::value, "");
+#endif
----------------
devin.jeanpierre wrote:
> Quuxplusone wrote:
> > I would say that in a perfect world `const TrivialMoveOnly` ought to be trivially relocatable, because `TrivialMoveOnly` is a trivially relocatable type, and if you're moving-and-destroying it, then you don't really care that the source object was const-qualified.
> > However, I see that even under p1144r5, `is_relocatable_v<const TrivialMoveOnly>` would be `false`, and I did not intend that `is_trivially_relocatable_v<T> && !is_relocatable_v<T>` should ever be true for any `T`. So I'll have to change this part of p1144. Most library users, then, should probably use `is_trivially_relocatable<remove_cvref_t<T>>` for their purposes.
> > 
> > Relevant question for this PR: Should we permit the libc++ extension type `std::vector<const X>` to actually `push_back` and reallocate when `is_trivially_relocatable_v<X>`? In the past that was obviously impossible, but now it's kinda possible.
> > Relevant question for this PR: Should we permit the libc++ extension type `std::vector<const X>` to actually `push_back` and reallocate when `is_trivially_relocatable_v<X>`? In the past that was obviously impossible, but now it's kinda possible.
> 
> My personal opinion is that this is too dangerous: people will rely on it by accident, not even knowing what they're doing (e.g. templated code with levels of indirection), and that would make it significantly harder to remove or change `__is_trivially_relocatable`.
> 
> Separately (bit of a cop-out), I think that should be a separate PR: it's a new feature, whereas this PR is "just" an optimization, if that makes sense.
> 
> I would say that in a perfect world `const TrivialMoveOnly` ought to be trivially relocatable, because `TrivialMoveOnly` is a trivially relocatable type, and if you're moving-and-destroying it, then you don't really care that the source object was const-qualified.

Yeah, it is a bit unfortunate. FWIW I think the correct behavior right now is that it should not be trivially relocatable, as it is not trivially move constructible. If we want to make it considered trivially relocatable, we should also suitably alter the definition of triviality elsewhere -- I think, rather than searching for eligible constructors, the definition would search for eligible constructors without const qualification.

(No idea for volatile, I try not to think about it.)

> Relevant question for this PR: Should we permit the libc++ extension type `std::vector<const X>` to actually `push_back` and reallocate when `is_trivially_relocatable_v<X>`? In the past that was obviously impossible, but now it's kinda possible.

My personal opinion is that this is too dangerous: people will rely on it by accident, not even knowing what they're doing (e.g. templated code with levels of indirection), and that would make it significantly harder to remove or change `__is_trivially_relocatable`.

Separately (bit of a cop-out), I think that should be a separate PR: it's a new feature, whereas this PR is "just" an optimization, if that makes sense.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385