Right now the variant::valueless_by_exception() calls !_M_valid() that is defined like that: _M_valid() const noexcept { return this->_M_index != __index_type(variant_npos); } Above code is quite complex for the optimizer. It may be profitable to detect variant types that never have valueless_by_exception() state and re-implement _M_valid: _M_valid() const noexcept { return __always_valid::value /*compile-time constant*/ || this->_M_index != __index_type(variant_npos); } Such change would improve assembly for multiple functions including comparisons, swap, get, constructors.
Can we do any better than this? --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -439,6 +439,8 @@ namespace __variant constexpr bool _M_valid() const noexcept { + if constexpr ((is_scalar_v<_Types> && ...)) + return true; return this->_M_index != __index_type(variant_npos); } Any non-scalar type can have a constructor that throws, which could be called via variant::emplace.
New intrinsic could be added into the GCC... something like __builtin_all_constructors_nothrow(type)
That makes me a little uncomfortable. Currently you can use something like: struct X { #ifndef NDEBUG X(int) noexcept(false); #endif //... }; bool test(std::variant<int, X> const& v) { return v.valueless_by_exception(); } and it "works" (despite the ODR violation), even if the variant was put into a valueless state in a TU where the throwing constructor was defined and the test is in a TU where it isn't defined. The suggested intrinsic would turn this from a "harmless" ODR violation to a more problematic one.
Author: redi Date: Tue Sep 25 14:59:16 2018 New Revision: 264574 URL: https://gcc.gnu.org/viewcvs?rev=264574&root=gcc&view=rev Log: PR libstdc++/87431 optimise valueless_by_exception() If a std::variant can never get into valueless state then we don't need to do a runtime check for a valid alternative. PR libstdc++/87431 * include/std/variant (_Variant_storage<true, _Types...>::_M_valid): Avoid runtime test when all alternatives are scalars and so cannot throw during initialization. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/std/variant
I'm going to mark this as fixed, I'm uncomfortable doing a smarter fix that would be more fragile.
(In reply to Jonathan Wakely from comment #5) > I'm going to mark this as fixed, I'm uncomfortable doing a smarter fix that > would be more fragile. +1. I'll keep searching for a less fragile solution.
This "optimization" might be unsafe in the presence of throwing conversion operator: // https://wandbox.org/permlink/ADEIfSv120QMrKeS #include <variant> #include <cassert> struct weird { operator int() { throw 0; } }; int main() { std::variant<char, int> v; try { v.emplace<1>(weird{}); } catch (...) { assert((v.index() == std::variant_npos) == (v.valueless_by_exception())); } } Maybe we can fix the optimization by making `emplace` restore the original value (or not destroy in the first place) when the optimization is present and an exception is thrown.
Here's an idea: Make valueless_by_exception() always false if all the alternatives are trivially copyable. Implement copying and constructions/emplace for such variants as a construction of a temporary + memcpy to destination. In theory the compiler should eliminate the unnecessary copy.
Sigh, I'm starting to dislike variant as much as tuple. Which is a lot. Reopening.
Maybe like this: --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -439,7 +439,7 @@ namespace __variant constexpr bool _M_valid() const noexcept { - if constexpr ((is_scalar_v<_Types> && ...)) + if constexpr ((is_trivially_copyable_v<_Types> && ...)) return true; return this->_M_index != __index_type(variant_npos); } @@ -1185,6 +1185,23 @@ namespace __variant { static_assert(_Np < sizeof...(_Types), "The index should be in [0, number of alternatives)"); + + using type = variant_alternative_t<_Np, variant>; + // If constructing the value can throw but move assigning it can't, + // construct in a temporary and then move assign from it. This gives + // the strong exception safety guarantee, ensuring we never become + // valueless. + if constexpr (is_trivially_copyable_v<type> + && !is_nothrow_constructible_v<type, _Args...>) + { + // If move assignment cannot throw then we can provide the + // strong exception safety guarantee, and never become valueless. + variant __tmp(in_place_index<_Np>, + std::forward<_Args>(__args)...); + *this = std::move(__tmp); + return std::get<_Np>(*this); + } + this->~variant(); __try { @@ -1208,6 +1225,20 @@ namespace __variant { static_assert(_Np < sizeof...(_Types), "The index should be in [0, number of alternatives)"); + + using type = variant_alternative_t<_Np, variant>; + if constexpr (is_trivially_copyable_v<type> + && !is_nothrow_constructible_v<type, initializer_list<_Up>, + _Args...>) + { + // If move assignment cannot throw then we can provide the + // strong exception safety guarantee, and never become valueless. + variant __tmp(in_place_index<_Np>, __il, + std::forward<_Args>(__args)...); + *this = std::move(__tmp); + return std::get<_Np>(*this); + } + this->~variant(); __try {
Looks good. Note that boost::variant went further: if all the types are nothrow movable then variant always does the trick with moving from temporary. In that way `valueless_by_exception()` like states never happen. Such approach may not fit the libstdc++.
Yeah that unconditional move might actually be more expensive than just constructing directly. E.g. if the move constructor isn't really cheap, or if it will memcpy a large object. If an exception is rare in practice, then it might be better to "risk" the exception by constructing directly. It's a trade off.
Yeah... but some whitelist of types to move could be hardcoded. For example std::basic_string, std::vector, std::unique_ptr and std::shared_ptr could be safely moved and `valueless_by_exception()` never happen for them. Those types cover some of the popular std::variant usages and the overhead from `valueless_by_exception()` will be avoided for those cases.
Author: redi Date: Sun Jan 6 20:52:34 2019 New Revision: 267614 URL: https://gcc.gnu.org/viewcvs?rev=267614&root=gcc&view=rev Log: PR libstdc++/87431 fix regression introduced by r264574 The previous patch for PR 87431 assumed that initialing a scalar type could not throw, but it can obtain its value via a conversion operator, which could throw. This meant the variant could get into a valueless state, but the valueless_by_exception() member function would always return false. This patch fixes it by changing the emplace members to have strong exception safety when initializing a contained value of trivially copyable type. The _M_valid() member gets a corresponding change to always return true for trivially copyable types, not just scalar types. Strong exception safety (i.e. never becoming valueless) is achieved by only replacing the current contained value once any potentially throwing operations have completed. If constructing the new contained value can throw then a new std::variant object is constructed to hold it, and then move-assigned to *this (which won't throw). PR libstdc++/87431 * include/std/variant (_Variant_storage<true, _Types...>::_M_valid): Check is_trivially_copyable instead of is_scalar. (variant::emplace<N, Args>(Args&&...)): If construction of the new contained value can throw and its type is trivially copyable then construct into a temporary variant and move from it, to provide the strong exception safety guarantee. (variant::emplace<N, U, Args>(initializer_list<U>, Args&&...)): Likewise. * testsuite/20_util/variant/87431.cc: New test. * testsuite/20_util/variant/run.cc: Adjust test so that throwing conversion causes valueless state. Added: trunk/libstdc++-v3/testsuite/20_util/variant/87431.cc Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/std/variant trunk/libstdc++-v3/testsuite/20_util/variant/run.cc
Should be fixed now.
Author: redi Date: Tue Jan 8 23:15:49 2019 New Revision: 267743 URL: https://gcc.gnu.org/viewcvs?rev=267743&root=gcc&view=rev Log: Pretty printer test fixes and improvements Test that StdUniquePtrPrinter correctly prints std::unique_ptr objects using the old layout, prior to the PR libstdc++/77990 changes. The printer test for a valueless std::variant started to fail because the PR libstdc++/87431 fix meant it no longer became valueless. Change the test to use a type that is not trivially copyable, so that the exception causes it to become valueless. * testsuite/libstdc++-prettyprinters/compat.cc: Test printer support for old std::unique_ptr layout. * testsuite/libstdc++-prettyprinters/cxx17.cc: Fix std::variant test to become valueless. Add filesystem::path tests. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/testsuite/libstdc++-prettyprinters/compat.cc trunk/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc
Here's a silly example which works with GCC 8 but fails with current trunk: #include <variant> struct S { S(int) : a() { } char a[8192 * 1024]; }; int main() { auto& v = *new std::variant<int, S>; v.emplace<S>(1); S &s = std::get<1>(v); return s.a[0]; } This blows my shell's default 8kb stack limit, because of the temporary that gets created on the stack. We could revert to having never-valueless depend on (is_scalar_v<_Types>&&...) or keep depending on is_trivially_copyable but also check that all the alternative types are smaller than some limit like 512 bytes.
Proposed new patch posted: https://gcc.gnu.org/ml/gcc-patches/2019-04/msg00142.html
Author: redi Date: Fri Apr 5 16:56:09 2019 New Revision: 270170 URL: https://gcc.gnu.org/viewcvs?rev=270170&root=gcc&view=rev Log: PR libstdc++/87431 re-adjust never-valueless optimizations Avoid creating arbitrarily large objects on the stack when emplacing trivially copyable objects into a variant. Currently we provide the strong exception-safety guarantee for all trivially copyable types, by constructing a second variant and then doing a non-throwing move assignment from the temporary. This patch restricts that behaviour to trivially copyable types that are no larger than 256 bytes. For larger types the object will be emplaced directly into the variant, and if its initialization throws then the variant becomes valueless. Also implement Antony Polukhin's suggestion to whitelist specific types that are not trivially copyable but can be efficiently move-assigned. Emplacing those types will never cause a variant to become valueless. The whitelisted types are: std::shared_ptr, std::weak_ptr, std::unique_ptr, std::function, and std::any. Additionally, std::basic_string, std::vector, and __gnu_debug::vector are whitelisted if their allocator traits give them a non-throwing move assignment operator. Specifically, this means std::string is whitelisted, but std::pmr::string is not. As part of this patch, additional if-constexpr branches are added for the cases where the initialization is known to be non-throwing (so the overhead of the try-catch block can be avoided) and where a scalar is being produced by a potentially-throwing conversion operator (so that the overhead of constructing and move-assigning a variant is avoided). These changes should have no semantic effect, just better codegen. PR libstdc++/87431 (again) * include/bits/basic_string.h (__variant::_Never_valueless_alt): Define partial specialization for basic_string. * include/bits/shared_ptr.h (_Never_valueless_alt): Likewise for shared_ptr and weak_ptr. * include/bits/std_function.h (_Never_valueless_alt): Likewise for function. * include/bits/stl_vector.h (_Never_valueless_alt): Likewise for vector. * include/bits/unique_ptr.h (_Never_valueless_alt): Likewise for unique_ptr. * include/debug/vector (_Never_valueless_alt): Likewise for debug vector. * include/std/any (_Never_valueless_alt): Define explicit specialization for any. * include/std/variant (_Never_valueless_alt): Define primary template. (__never_valueless): Use _Never_valueless_alt instead of is_trivially_copyable. (variant::emplace<N>(Args&&...)): Add special case for non-throwing initializations to avoid try-catch overhead. Add special case for scalars produced by potentially-throwing conversions. Use _Never_valueless_alt instead of is_trivially_copyable for the remaining strong exception-safety cases. (variant::emplace<N>(initializer_list<U>, Args&&...)): Likewise. * testsuite/20_util/variant/87431.cc: Run both test functions. * testsuite/20_util/variant/exception_safety.cc: New test. * testsuite/20_util/variant/run.cc: Use pmr::string instead of string, so the variant becomes valueless. Added: trunk/libstdc++-v3/testsuite/20_util/variant/exception_safety.cc Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/bits/basic_string.h trunk/libstdc++-v3/include/bits/shared_ptr.h trunk/libstdc++-v3/include/bits/std_function.h trunk/libstdc++-v3/include/bits/stl_vector.h trunk/libstdc++-v3/include/bits/unique_ptr.h trunk/libstdc++-v3/include/debug/vector trunk/libstdc++-v3/include/std/any trunk/libstdc++-v3/include/std/variant trunk/libstdc++-v3/testsuite/20_util/variant/87431.cc trunk/libstdc++-v3/testsuite/20_util/variant/run.cc
And fixed, again.
Sigh, there is yet another problem with this patch. Consider: #include <variant> #include <vector> struct DeletedMoves { DeletedMoves() = default; DeletedMoves(const DeletedMoves&) = default; DeletedMoves(DeletedMoves&&) = delete; DeletedMoves& operator=(const DeletedMoves&) = default; DeletedMoves& operator=(DeletedMoves&&) = delete; }; struct ThrowingCopy { ThrowingCopy() = default; ThrowingCopy(const ThrowingCopy&) { throw 1; } ThrowingCopy& operator=(const ThrowingCopy&) { throw 1; } }; int main() { using namespace std; variant<int, DeletedMoves, vector<ThrowingCopy>> v; v.emplace<2>(1); v.emplace<2>(1); } _Never_valueless<vector<ThrowingCopy>> is true, so we try to provide the strong-exception guarantee: else if constexpr (__detail::__variant::_Never_valueless_alt<type>()) { // This construction might throw: variant __tmp(in_place_index<_Np>, __il, std::forward<_Args>(__args)...); // But _Never_valueless_alt<type> means this won't: *this = std::move(__tmp); } However because is_move_assignable_v<DeletedMoves> is false the variant has no move assignment operator, only a copy assignment operator. That means the "move" assignment actually uses the variant's copy assignment operator, which performs a copy of the vector, and that can throw. If the variant's current contained value is not the same type as the one being emplaced, the copy assignment will destroy the previous value, and then if copying the vector throws we become valueless. But the variant thinks it can never become valueless, so we have undefined behaviour. We can restrict the strong exception safety guarantee to only happen when the variant has a move assignment operator: --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -1428,7 +1428,8 @@ namespace __variant this->_M_reset(); __variant_construct_by_index<_Np>(*this, __tmp); } - else if constexpr (__detail::__variant::_Never_valueless_alt<type>()) + else if constexpr (__detail::__variant::_Never_valueless_alt<type>() + && _Traits::_S_move_assign) { // This construction might throw: variant __tmp(in_place_index<_Np>, @@ -1474,7 +1475,8 @@ namespace __variant __variant_construct_by_index<_Np>(*this, __il, std::forward<_Args>(__args)...); } - else if constexpr (__detail::__variant::_Never_valueless_alt<type>()) + else if constexpr (__detail::__variant::_Never_valueless_alt<type>() + && _Traits::_S_move_assign) { // This construction might throw: variant __tmp(in_place_index<_Np>, __il, But this means that branch won't be taken for the variant type in the example above, and so we don't offer the strong exception safety guarantee for that type, so we also need to consider the extra condition in the __never_valueless() function: @@ -352,7 +352,8 @@ namespace __variant template <typename... _Types> constexpr bool __never_valueless() { - return (_Never_valueless_alt<_Types>::value && ...); + return _Traits<_Types...>::_S_move_assign + && (_Never_valueless_alt<_Types>::value && ...); } // Defines index and the dtor, possibly trivial.
I'm tempted to just rip out this stuff entirely, and go back to only offering the strong exception safety guarantee for trivially copyable types, and so variants would only be never-valueless if all alternatives are trivially copyable.
Author: redi Date: Tue Apr 23 09:55:33 2019 New Revision: 270502 URL: https://gcc.gnu.org/viewcvs?rev=270502&root=gcc&view=rev Log: Fix std::variant regression caused by never-valueless optimization A regression was introduced by the recent changes to provide the strong exception safety guarantee for "never valueless" types that have O(1), non-throwing move assignment. The problematic code is: else if constexpr (__detail::__variant::_Never_valueless_alt<type>()) { // This construction might throw: variant __tmp(in_place_index<_Np>, __il, std::forward<_Args>(__args)...); // But _Never_valueless_alt<type> means this won't: *this = std::move(__tmp); } When the variant is not assignable, the assignment is ill-formed, so should not be attempted. When the variant has a copy assignment operator but not a move assignment operator, the assignment performs a copy assignment and that could throw, so should not be attempted. The solution is to only take that branch when the variant has a move assignment operator, which is determined by the _Traits::_S_move_assign constant. When that is false the strong exception safety guarantee is not possible, and so the __never_valueless function should also depend on _S_move_assign. While testing the fixes for this I noticed that the partial specialization _Never_valueless_alt<basic_string<C,T,A>> incorrectly assumed that is_nothrow_move_constructible<basic_string<C,T,A>> is always true, but that's wrong for fully-dynamic COW strings. Fix the partial specialization, and improve the comment describing _Never_valueless_alt to be clear it depends on move construction as well as move assignment. Finally, I also observed that _Variant_storage<false, T...>::_M_valid() was not taking advantage of the __never_valueless<T...>() function to avoid a runtime check. Only the _Variant_storage<true, T...>::_M_valid() function was using __never_valueless. That is also fixed. PR libstdc++/87431 * include/bits/basic_string.h (_Never_valueless_alt): Make partial specialization also depend on is_nothrow_move_constructible. * include/std/variant (__detail::__variant::__never_valueless()): Only true if the variant would have a move assignment operator. (__detail::__variant::_Variant_storage<false, T...>::_M_valid()): Check __never_valueless<T...>(). (variant::emplace): Only perform non-throwing move assignments for never-valueless alternatives if the variant has a move assignment operator. * testsuite/20_util/variant/compile.cc: Check that never-valueless types can be emplaced into non-assignable variants. * testsuite/20_util/variant/run.cc: Check that never-valueless types don't get copied when emplaced into non-assignable variants. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/bits/basic_string.h trunk/libstdc++-v3/include/std/variant trunk/libstdc++-v3/testsuite/20_util/variant/compile.cc trunk/libstdc++-v3/testsuite/20_util/variant/run.cc
Fixed again, I hope.