[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
libcxx-commits at lists.llvm.org
Fri Mar 18 14:18:46 PDT 2022
devin.jeanpierre added a comment.
In D119385#3392540 <https://reviews.llvm.org/D119385#3392540>, @ldionne wrote:
> @devin.jeanpierre Would you be willing to get on a call with me to go over your patch? I think it would make it easier to make progress -- you have significantly more context than me around it, so it'd be helpful for me.
Almost missed this request -- absolutely, I'd be happy to. I don't know the procedure around this stuff though.
================
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:
> devin.jeanpierre wrote:
> > 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.
> I continue to not understand why we want to make the caller aware of this. Take for example `__construct_backward_with_exception_guarantees`. it's always called with the same `__move_via_memcpy()` argument -- why not define that condition in `enable_if_t` in the function itself instead? Sorry if it's obvious to you, it's not to me.
>
> Another way to see this: the `true_type` vs `false_type` is just a transparent optimization -- the semantics of `__construct_backward_with_exception_guarantees` are the same in both cases (right?). So the caller effectively doesn't need to know whether the optimization kicks in if `__construct_backward_with_exception_guarantees` can determine it alone. And if I'm wrong and `true_type` vs `false_type` does change the semantics, then we should have another function with a different name instead, because both would do different things.
So it definitely does change semantics: if you call `__construct_backward_with_exception_guarantees` with `true_type`, but the object is not trivially-movable, then you are promising that 1) it is trivially relocatable (via `[[clang::trivial_abi]]`, today), and 2) you will be releasing the underlying storage of the "moved"-from objects without invoking the destructor.
The compiler can infer (1): it could just check for trivially relocatability. That's what the callers do! But it cannot check for (2), the caller has to be careful here. So I do think it should probably be explicit.
If I were to do it myself, I'd probably pass in `reinterpret_cast<const char*>(my_pointer)`, rather than defining a separate function. The benefit of having it be a true_type/false_type argument is, I suppose, that it's easy to call: it's just a parameter as to whether to use trivial relocation, and the caller can do neat so that they have just one call site with a varying parameter. I can see the merit to it. For example, one caller looks like so:
```
__construct_forward_with_exception_guarantees(this->__alloc(), __p, this->__end_, __v.__end_, __move_via_memcpy());
```
If we were to make it different functions, or require a reinterpret_cast in the caller, it would become:
```
if (__move_via_memcpy::value) {
__trivial_construct_forward_with_exception_guarantees(this->__alloc(), __p, this->__end_, __v.__end__;
// or: __construct_forward_with_exception_guarantees(this->__alloc(), reinterpret_cast<const char*(...), reinterpret_cast<const char*(...), reinterpret_cast<const char*(...);
} else {
__construct_forward_with_exception_guarantees(this->__alloc(), __p, this->__end_, __v.__end__;
}
```
So I can see the merit to it.
Of course, the magic boolean parameter seemed weird to me too. It was impressed on me when I asked a coworker that it is not so strange, and indeed cases already exist of it in llvm: for example, `__move_assign` in vector, `__process` in memory, etc.
So what I want to know is -- is this really that weird, and existing code is "legacy" code we should try not to emulate? Or is this relatively normal? I will do whatever the "normal" thing is. :P
Leaving your comment unresolved. :)
================
Comment at: libcxx/include/memory:871
-template <class _Alloc, class _Tp, typename enable_if<
- (__is_default_allocator<_Alloc>::value || !__has_construct<_Alloc, _Tp*, _Tp>::value) &&
- is_trivially_move_constructible<_Tp>::value
->::type>
+template <class _Alloc, class _Ptr>
_LIBCPP_INLINE_VISIBILITY
----------------
ldionne wrote:
> This should be `class _ContiguousIterator` to document that effectively any contiguous iterator is what we expect? I assume this was changed from `_Tp*` because we had to support `__wrap_iter`?
(I confess I'm not sure why this had to be changed. I can only assume because now it's selected in branches that, yes, use __wrap_iter, but it's very indirect.)
FWIW _Ptr was already in use in the other overload, so I would prefer to leave it unchanged for now. LMK if I should change both, or if it's acceptable/preferable to change just this one, and I'll do so.
================
Comment at: libcxx/include/memory:878
if (_Np > 0) {
- _VSTD::memcpy(__begin2, __begin1, _Np * sizeof(_Tp));
+ _VSTD::memcpy(const_cast<_Vp*>(_VSTD::__to_address(__begin2)), _VSTD::__to_address(__begin1), _Np * sizeof(_Tp));
__begin2 += _Np;
----------------
philnik wrote:
> You can replace `_VSTD` with `std` in your changes. Why are you `const_cast`ing now?
The `const_cast` is to support `vector<const T>`, see e.g. libcxx/test/libcxx/containers/sequences/vector/const_value_type.pass.cpp.
I thought libc++ no longer supported const-qualified value types, but it seems that test is still there, and the change deleting it was rolled back. So until libc++ properly removes this, I need to retain the const_casting after all. Oops!
Note that this isn't "new" -- e.g. line 902 of the LHS revision at time of writing also does a const_cast for the same reason. However, I added a comment anyway, to avoid confusion.
Did my best to replace _VSTD with std.
================
Comment at: libcxx/include/type_traits:3127-3131
+#if _LIBCPP_STD_VER > 14
+template <class _Tp>
+inline constexpr bool __libcpp_is_trivially_relocatable_v
+ = __libcpp_is_trivially_relocatable<_Tp>::value;
+#endif
----------------
ldionne wrote:
> I'd rather not define this at all, since it's an internal only type trait. Concretely, it's less complicated to say `::value` everywhere than to check if we're in C++17 and use `_v` instead.
That makes sense to me. In point of fact, I was thinking the same thing!
================
Comment at: libcxx/include/vector:310
+ __has_default_allocator_construct<_Allocator, _Tp, _Tp&&>::value &&
+ !is_volatile<_Tp>::value
+> {};
----------------
philnik wrote:
> Every standard library agrees that it's not allowed to have a cv-qualified type as the template parameter for a vector (https://godbolt.org/z/8xEnhEbYT). Ditto below.
Deleted, and with joy in my heart.
Unfortunately I then proactively did the same for const, because I thought that change had gone in -- but the change deleting const-qualification support in libc++ was rolled back.
================
Comment at: libcxx/include/vector:695-696
void __move_range(pointer __from_s, pointer __from_e, pointer __to);
+ template<class _Up>
+ void __relocate_upward_and_insert(pointer __from_s, pointer __from_e, _Up&& __elt);
void __move_assign(vector& __c, true_type)
----------------
philnik wrote:
> Do we want this function in the dylib?
Unfortunately I'm not sure what this is asking, and probably don't have the background to give an informed answer. Anyone else want to step in here?
================
Comment at: libcxx/include/vector:900-903
+ typedef integral_constant<bool,
+ __vector_move_via_memcpy<_Tp, _Allocator>::value ||
+ __vector_relocate_via_memcpy<_Tp, _Allocator>::value
+ > __move_via_memcpy;
----------------
philnik wrote:
> That looks very weird. Why is `__move_via_memcpy` move via memcpy or relocate via memcpy? It looks like you are using `__vector_move_via_memcpy` only in two places and both do this weird thing. Why not change `__vector_move_via_memcpy`? The naming for the second typedef looks also weird. (And you can use `using` instead)
Yeah, I agree. WDYT about how I've done it now? Three categories:
* trivial_relocate: relocation operations can be trivial (both move+destroy), because the type is trivially relocatable
* relocate_trivial_move: relocation operations use memcpy -- maybe because it's trivially relocatable, maybe because it's trivially movable
* relocate_trivial_destroy: relocation operations drop underlying storage without invoking the destructor -- maybe because it's trivially relocatable, maybe because (in a future change) it's trivially destructible?
I've made the corresponding changes to the source so you can see how it plays out.
================
Comment at: libcxx/include/vector:1644-1650
+ if (__vector_relocate_via_memcpy<_Tp, _Allocator>::value) {
+ _Tp *__rawp = _VSTD::__to_address(__p);
+ __alloc_traits::destroy(this->__alloc(), __rawp);
+ --this->__end_;
+ if (__p != this->__end_) {
+ _VSTD::memmove(__rawp, __rawp + 1, (this->__end_ - __p) * sizeof(*__rawp));
+ }
----------------
philnik wrote:
> Is `this->` needed? If not, please remove it.
Ah, sorry, I was just doing whatever the code already did. Looks like it was unnecessary.
================
Comment at: libcxx/include/vector:1649
+ if (__p != this->__end_) {
+ _VSTD::memmove(__rawp, __rawp + 1, (this->__end_ - __p) * sizeof(*__rawp));
+ }
----------------
philnik wrote:
> Could you use `std::move` here? It should behave exactly the same and is easier better to read.
(Please forgive me if I'm misunderstanding something here.)
Not without casting to e.g. std::byte pointers first -- that would invoke move constructors, where here we should be copying raw bytes.
The difference in behavior comes up if you have a nontrivial but `clang::trivial_abi` type, such as:
```
struct [[clang::trivial_abi]] MyClass {
MyClass(const MyClass&) { ... }
MyClass(MyClass&&) { ... }
~MyClass() {}
};
```
The intention of this change is to use `memmove` for this type, and not invoke move constructors etc.. So we skip across `std::move` and instead use memmove.
We also cannot move this optimization to `std::move`, because in general, `std::move` should still be calling move constructors: it is only in the specific case that the moved-from location is destroyed immediately afterwards that we can safely use `memcpy`/`memmove`: we can replace a move+destroy pair with a relocate + release-underlying-storage pair.
(That's not quite what's happening with erase, but we could imagine it was: erase is, I believe, allowed to work as if it moved everything after the erased point to a temporary buffer, destroyed the original objects, and then moved them back, destroying the temporary buffer.)
Hoping that addresses your comment!
================
Comment at: libcxx/include/vector:1684
+struct __is_nothrow_constructible_a : integral_constant<bool,
+ noexcept(allocator_traits<_Allocator>::construct(declval<_Allocator&>(), (_Tp*)nullptr, declval<_Up>()))
+> {};
----------------
philnik wrote:
> `noexcept` isn't available in C++03 AFAIK.
This is in the `#else` block, so that's fine, right? (Should only fire for C++11 and up, since e.g. 98 isn't supported.)
================
Comment at: libcxx/include/vector:1689
+template<class _Tp, class _Up>
+struct __is_nothrow_constructible_a<allocator<_Tp>, _Tp, _Up> : is_nothrow_constructible<_Tp, _Up> {};
+
----------------
philnik wrote:
> Could you rename this to something like `__is_nothrow_constructible_alloc`? `__is_nothrow_constructible_a` sounds a lot like you needed another name and you just slapped `_a` at the end.
> Do you only have this to allow better optimizations in C++03? If that is the case I'd say just remove it. If you care about performace you should switch to C++11 or newer. We currently don't even enable the move overloads in C++03 in most (all?) classes.
> Could you rename this to something like `__is_nothrow_constructible_alloc`? `__is_nothrow_constructible_a` sounds a lot like you needed another name and you just slapped `_a` at the end.
Agreed, and done!
> Do you only have this to allow better optimizations in C++03? If that is the case I'd say just remove it. If you care about performace you should switch to C++11 or newer. We currently don't even enable the move overloads in C++03 in most (all?) classes.
Hmmm, correct me if I'm wrong, but I think it's the `#else` block that doesn't do anything. These two lines are basically equivalent:
```
struct __is_nothrow_constructible_a : integral_constant<bool, false> {};
struct __is_nothrow_constructible_a : integral_constant<bool, noexcept(allocator_traits<_Allocator>::construct(declval<_Allocator&>(), (_Tp*)nullptr, declval<_Up>()))> {};
```
... because `allocator_traits<T>::construct` is never noexcept unless you specialize it, which libc++ does not permit.
The intent of the check is clearly that the `::construct` call will not throw, which we can determine, instead, by if the allocator defines construct at all. (Since we don't support overloading the trait).
Something like: `is_nothrow_constructible<_Tp, _Up>::value && (__is_default_allocator<_Allocator>::value || !__has_construct<_Allocator, _Tp*, _Tp>::value`
Does that look good?
================
Comment at: libcxx/test/libcxx/containers/sequences/vector/insert_trivially_relocatable.pass.cpp:13
+
+// UNSUPPORTED: c++98, c++03
+
----------------
philnik wrote:
> We don't support C++98. Ditto for the other tests
Apologies, I think earlier reviewers made the same comment and I accidentally failed to address it. My bad.
================
Comment at: libcxx/test/libcxx/containers/sequences/vector/insert_trivially_relocatable.pass.cpp:19
+
+static int assignments = 0;
+
----------------
philnik wrote:
> Could you make this into a local variable? These tests have to work during constant evaluation in the (hopefully near) future.
Not super sure what is allowed in constant evaluation, but hoping this is what you meant. At the very least, I believe it's an improvement.
================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> ldionne wrote:
> > philnik wrote:
> > > This looks like the wrong license header.
> > Yeah, this must have been before the LLVM re-licensing. Please make sure all your files are using the new license.
> IIUC we're not adding `std::is_trivially_relocatable`, so this test should go away entirely.
Well, but we are adding `__libcpp_is_trivially_relocatable`. Perhaps this should go in a separate change, though, since morally speaking it's testing `__is_trivially_relocatable`? WDYT?
================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:1-8
+//===----------------------------------------------------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
----------------
devin.jeanpierre wrote:
> ldionne wrote:
> > ldionne wrote:
> > > philnik wrote:
> > > > This looks like the wrong license header.
> > > Yeah, this must have been before the LLVM re-licensing. Please make sure all your files are using the new license.
> > IIUC we're not adding `std::is_trivially_relocatable`, so this test should go away entirely.
> Well, but we are adding `__libcpp_is_trivially_relocatable`. Perhaps this should go in a separate change, though, since morally speaking it's testing `__is_trivially_relocatable`? WDYT?
Apologies, I didn't even think to check for that! Done -- looks like it was just the two tests.
================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:56-72
+struct T {
+ T();
+ T(const T&);
+ ~T();
+};
+struct R {};
+struct SM {
----------------
philnik wrote:
> Could you give these things a more meaningful name?
Agreed, and done. :)
Actually, I hadn't looked closely at this test. I think it can be simplified a bit too -- for example, rather than having a separate nontrivial key type, I just added operator== etc. to the nontrivial type. (Could do the same for the mutex type, but wasn't sure if that'd be even more confusing.)
================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:78-83
+static const bool NotDebug =
+#if _LIBCPP_DEBUG_LEVEL >= 2
+ false;
+#else
+ true;
+#endif
----------------
philnik wrote:
>
Ooh, indeed.
{F22479517}
================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:85-86
+
+// Define this name for convenience.
+#define is_trivially_relocatable __libcpp_is_trivially_relocatable
+
----------------
philnik wrote:
> Please don't define this kind of stuff for convenience. What if in a new standard there will be `std::is_trivially_relocatable`? That's not that unlikely IMO and then we would have to rewrite this thing just because you felt like adding a macro here. It's not hard to replace the uses, so you can do it now.
You are 100% right, done.
================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.pass.cpp:16
+// UNSUPPORTED: c++98, c++03
+// XFAIL: gcc-4.9
+
----------------
philnik wrote:
> We don't support gcc-4.9.
Apologies, and thanks for the correction!
================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.pass.cpp:94
+ return 0;
+}
----------------
philnik wrote:
> You can make this file a `.compile.pass.cpp`.
Good call, thank you!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119385/new/
https://reviews.llvm.org/D119385
More information about the libcxx-commits
mailing list