[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 Apr 28 21:14:56 PDT 2022

devin.jeanpierre added a comment.

Really sorry about the delay on this one, I stepped away to work on a build breakage <https://reviews.llvm.org/rGf2b31f06b79a6cfb7eb3146dfc1d514da52142e9> for the predecessor PR, and then never came back to this. (And then I felt bad about never coming back to this, and... well, maybe you know how it is, "ugh field" etc.)  I'll try to stay more on top of this, since I definitely want it to go in! I'm really excited about this change. :)

Let me know how to proceed. And, obviously, feel free to take your time, I don't have the right at this point to request speedy review.

================
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:
> 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.
I'm still not following. My understanding had been that this should be fine, because `noexcept` is only used if `_LIBCPP_CXX03_LANG` is not defined, meaning that we only use `noexcept` on C++11 and above.

I think this is no longer relevant though, since that code snippet is deleted.

================
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:
> 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.)
It's more complicated than that, because the allocator is allowed not to define construct -- so we need to do the dance that __has_construct does to ensure that this fails gracefully (SFINAE) when it doesn't exist.

Can we defer this / not implement it now?  I played around with this a bit, but as it turns out I'm really not very familiar with template metaprogramming in C++. If this is a strongly desired feature, I can write a followup as my very next change, but otherwise I'd actually prefer not to write it at all until a concrete use presents itself. :)

(I think such concrete uses may not exist at all, or be very rare -- it'd be a construct function that is defined only for types with noexcept constructors, which is I think impossible to guarantee in C++ (you can't enumerate all the constructors). So the only allocators that would satisfy this are those that are special-cased to known types to be noexcept, or which are noexcept even though the underlying constructors might throw.)

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

================
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, "");
----------------
philnik wrote:
> 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?
Great catch.

Um, actually, on reflection, this whole file is a nightmare of test failures depending on build mode. My experience with the predecessor PR D114732 is that this will be very hard to get right -- we will need ifdefs for win64 and for PS3, at the least -- not to mention, as you've noticed, checking for the unstable ABI.

Moreover, I think it doesn't really belong in this change. So at least for now, I'm reverting it entirely. If this change should go in, I think a separate PR would be best. (It's basically testing D114732, and possibly not this change.)

================
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, "");
----------------
philnik wrote:
> Do you have any interest in making a follow-up PR for these? It should be possible to make these all `trivially_relocatable`, right?
Yes, I think we should put work into making many common datatypes [[clang::trivial_abi]], in the same way as already done for unique_ptr, and I want to do this for at least a couple common/easy ones (hoping std::vector, std::string qualify).

Others might require more substantial design work to make this happen (e.g. the map types), and it might be that yet others fundamentally can't (e.g. std::list -- something in there is going to be inherently non-trivially-relocatable, since it's self-referential by design).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385