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

Sun Mar 20 18:59:18 PDT 2022

philnik added a comment.

This looks good so far. I think with Louis' and my comments addressed this should be good to go, but I want to do another thorough review with the comments addressed before approving this.

================
Comment at: libcxx/include/vector:1707
+        __move_range(__from_s, __from_e, __from_s + 1);
+        *__from_s = static_cast<_Up&&>(__elt);
+    }
----------------
Is this a forward? If yes, please spell it as one.

================
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)
----------------
devin.jeanpierre wrote:
> 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?
I think that's a question @ldionne has to answer. My question is essentially if we want to have this as part of the ABI or if it should be marked `_LIBCPP_HIDE_FROM_ABI`.

================
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;
----------------
devin.jeanpierre wrote:
> 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.
Yes, looks much better!

================
Comment at: libcxx/include/vector:1649
+        if (__p != this->__end_) {
+            _VSTD::memmove(__rawp, __rawp + 1, (this->__end_ - __p) * sizeof(*__rawp));
+        }
----------------
devin.jeanpierre wrote:
> 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!
Right, I missed that it's implicitly casting to void* in the call to `std::memmove`. Using `std::move` would look something like
```
std::move(reinterpret_cast<byte*>(__rawp + 1), reinterpret_cast<byte*>(__rawp + (__end_ - __p) * sizeof(_Tp)), __rawp);
```
. That's obviously not an improvement. Currently I also don't have any good idea to make it more readable.

================
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>()))
+> {};
----------------
devin.jeanpierre wrote:
> 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.)
I think you misunderstood me a bit. We //don't// support C++98, but we //do// support C++03. I know there isn't a huge difference, but it's significant enough in a standard library implementation to point out. Although the C++03 support is very weird, but that's another story.

================
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> {};
+
----------------
devin.jeanpierre wrote:
> 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?
It definitely makes sense to me. Maybe you could check if the `allocator::construct` member function is `noexcept`. That would include more allocators. Something like `!__has_construct<_Allocator, _Tp*, _Tp>::value || __nothrow_invocable<_Allocator::construct, _Tp*, _Tp>::value` should work. (I haven't tested it.)

================
Comment at: libcxx/test/libcxx/containers/sequences/vector/insert_trivially_relocatable.pass.cpp:19
+
+static int assignments = 0;
+
----------------
devin.jeanpierre wrote:
> 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.
Yes, that's exactly what I had in mind.

================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:94-96
+static_assert(!std::__libcpp_is_trivially_relocatable<std::unique_ptr<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::shared_ptr<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::weak_ptr<Nontrivial>>::value, "");
----------------
Why aren't these trivially relocatable? These should be in the unstable ABI because they are marked `[[clang::trivial_abi]]`, or am I not understanding something?

================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:116-131
+static_assert(!std::__libcpp_is_trivially_relocatable<std::deque<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::forward_list<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::list<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::map<Nontrivial, Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::multimap<Nontrivial, Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::multiset<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::set<Nontrivial>>::value, "");
----------------
Do you have any interest in making a follow-up PR for these? It should be possible to make these all `trivially_relocatable`, right?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385