On 16/05/19 12:43 +0100, Jonathan Wakely wrote:
On 16/05/19 12:29 +0100, Jonathan Wakely wrote:
These two changes both result in smaller code for std::variant.
The first one means smaller tables of function pointers, because we
don't generate an instantiation for the valueless state. Instead we do
a runtime branch, marked [[unlikely]] to make _M_reset() a no-op if
it's already valueless. In a microbenchmark I couldn't measure any
performance difference due to the extra branch, so the code size
reduction seems worthwhile.
The second one removes a branch from the index() member by relying on
unsigned arithmetic. That also results in smaller code and I can't see
any downside.
* include/std/variant (_Variant_storage<false, _Types...>::_M_reset):
Replace raw visitation with a runtime check for the valueless state
and a non-raw visitor.
(_Variant_storage<false, _Types...>::_M_reset_impl): Remove.
(variant::index()): Remove branch.
We might also want:
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -1503,7 +1503,7 @@ namespace __variant
}
else
{
- if (this->index() != variant_npos)
+ if (!this->valueless_by_exception()) [[__likely__]]
{
auto __tmp(std::move(__rhs_mem));
__rhs = std::move(*this);
@@ -1520,7 +1520,7 @@ namespace __variant
}
else
{
- if (this->index() != variant_npos)
+ if (!this->valueless_by_exception()) [[__likely__]]
{
__rhs = std::move(*this);
this->_M_reset();
This results in smaller code too, because for some specializations
valueless_by_exception() always returns false, so the branch can be
removed.
(This suggests that it's generally better to ask the yes/no question
"are you valid?" rather than "what is your index, and does it equal
this magic number?")
For specializations where a valueless state is possible we still
expect it to be very unlikely in practice, so the attribute should
help there.
Here's what I've tested and am about to commit.
commit d4e4bd9e53d81f410a8ae289d3b67d0295f8da96
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Wed May 15 22:33:31 2019 +0100
Changes to std::variant to reduce code size
* include/std/variant (_Variant_storage<false, _Types...>::_M_reset):
Replace raw visitation with a runtime check for the valueless state
and a non-raw visitor.
(_Variant_storage<false, _Types...>::_M_reset_impl): Remove.
(variant::index()): Remove branch.
(variant::swap(variant&)): Use valueless_by_exception() instead of
comparing the index to variant_npos, and add likelihood attribute.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 8c710c30de5..101b8945943 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -396,19 +396,16 @@ namespace __variant
_M_index(_Np)
{ }
- constexpr void _M_reset_impl()
- {
- __variant::__raw_visit([](auto&& __this_mem) mutable
- {
- if constexpr (!is_same_v<remove_reference_t<decltype(__this_mem)>,
- __variant_cookie>)
- std::_Destroy(std::__addressof(__this_mem));
- }, __variant_cast<_Types...>(*this));
- }
-
void _M_reset()
{
- _M_reset_impl();
+ if (!_M_valid()) [[unlikely]]
+ return;
+
+ std::__do_visit<void>([](auto&& __this_mem) mutable
+ {
+ std::_Destroy(std::__addressof(__this_mem));
+ }, __variant_cast<_Types...>(*this));
+
_M_index = variant_npos;
}
@@ -1485,12 +1482,7 @@ namespace __variant
{ return !this->_M_valid(); }
constexpr size_t index() const noexcept
- {
- if (this->_M_index ==
- typename _Base::__index_type(variant_npos))
- return variant_npos;
- return this->_M_index;
- }
+ { return size_t(typename _Base::__index_type(this->_M_index + 1)) - 1; }
void
swap(variant& __rhs)
@@ -1511,7 +1503,7 @@ namespace __variant
}
else
{
- if (this->index() != variant_npos)
+ if (!this->valueless_by_exception()) [[__likely__]]
{
auto __tmp(std::move(__rhs_mem));
__rhs = std::move(*this);
@@ -1528,7 +1520,7 @@ namespace __variant
}
else
{
- if (this->index() != variant_npos)
+ if (!this->valueless_by_exception()) [[__likely__]]
{
__rhs = std::move(*this);
this->_M_reset();
