Changes to std::variant to reduce code size

Message ID 20190516112901.GA28019@redhat.com
State New
Headers show
Series
  • Changes to std::variant to reduce code size
Related show

Commit Message

Jonathan Wakely May 16, 2019, 11:29 a.m.
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.

Tested powerpc64le-linux, any objections?
commit a36ecd71f2e7ce95b479d4c06597f2ee0dfe27b1
Author: Jonathan Wakely <jwakely@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.

Comments

Jonathan Wakely May 16, 2019, 11:43 a.m. | #1
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.
Jonathan Wakely May 16, 2019, 8:28 p.m. | #2
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 <jwakely@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();
Ville Voutilainen May 16, 2019, 8:31 p.m. | #3
On Thu, 16 May 2019 at 23:28, Jonathan Wakely <jwakely@redhat.com> wrote:
> Here's what I've tested and am about to commit.


Looks good to me.

Patch

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 8c710c30de5..8844c54913f 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)