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

Fri Mar 18 08:47:40 PDT 2022

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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

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

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

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

================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
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.

================
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.
+//
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385