Re: [boost] [optional] generates unnessesary code for trivial types

Subject: Re: [boost] [optional] generates unnessesary code for trivial types
From: Kim Barrett (kab.conundrums_at_[hidden])
Date: 2012-01-25 17:22:34

On Jan 25, 2012, at 12:28 PM, Hite, Christopher wrote:
> When decompiling my code I noticed a bunch of unnessesary code caused by boost::optional.

I happen to have been looking at the source and generated code for boost::optional recently myself, so jumping in here with a few comments.

> 1) deconstruction
> typedef boost::optional<int> optional_int;
> 
> void deconstruct_boost_optional(optional_int& o){
>        o.~optional_int();
> }
> 
> One would expect this to do nothing.  Instead gcc 4.6.0  with O3 generates:
> 
>        if(m_initialized){
>                // do nothing
>                m_initialized = false;
>        }
> 
> 00000000 <deconstruct_boost_optional(boost::optional<int>&)>:
>   0:   8b 44 24 04             mov    0x4(%esp),%eax
>   4:   80 38 00                cmpb   $0x0,(%eax)
>   7:   74 03                   je     c <deconstruct_boost_optional(boost::optional<int>&)+0xc>
>   9:   c6 00 00                movb   $0x0,(%eax)
>   c:   f3 c3                   repz ret
> 
> 
> This one could be easily fixed by removing the bit that sets m_initialized to false, since we're deconstructing anyway.

This sounds right to me.  Note that eliminating the assignment of m_initialized would (in this case of a trivial destructor for T) make the entire clause controlled by the conditional be empty after optimization, allowing the compiler to optimize away the conditional too.

What's going on here is that the destructor is calling the destroy() helper function, which does more work than the destructor actually needs, specifically setting m_initialized to false.  Other callers of destroy() do need that assignment.

> 3) Even more expensive is if we want to copy an optional<int>
> 
> void assign_boost_optional(optional_int& a,optional_int& b){
>        a=b;
> }
> 
> 00000000 <assign_boost_optional(boost::optional<int>&, boost::optional<int>&)>:
>   0:   8b 44 24 04             mov    0x4(%esp),%eax
>   4:   8b 54 24 08             mov    0x8(%esp),%edx
>   8:   80 38 00                cmpb   $0x0,(%eax)
>   b:   74 0b                   je     18 <assign_boost_optional(boost::optional<int>&, boost::optional<int>&)+0x18>
>   d:   80 3a 00                cmpb   $0x0,(%edx)
>  10:   75 16                   jne    28 <assign_boost_optional(boost::optional<int>&, boost::optional<int>&)+0x28>
>  12:   c6 00 00                movb   $0x0,(%eax)
>  15:   c3                      ret
>  16:   66 90                   xchg   %ax,%ax
>  18:   80 3a 00                cmpb   $0x0,(%edx)
>  1b:   74 09                   je     26 <assign_boost_optional(boost::optional<int>&, boost::optional<int>&)+0x26>
>  1d:   8b 52 04                mov    0x4(%edx),%edx
>  20:   c6 00 01                movb   $0x1,(%eax)
>  23:   89 50 04                mov    %edx,0x4(%eax)
>  26:   f3 c3                   repz ret
>  28:   8b 52 04                mov    0x4(%edx),%edx
>  2b:   89 50 04                mov    %edx,0x4(%eax)
>  2e:   c3                      ret
> 
> Three possible branches!  Theoretically single 64 bit copy do the job.  I'm tempted to say: it would be best if for any T has_trivial_copy< optional<T> > iff has_trivial_copy<T>.  It might make a sense to make an exception for huge T, where the copying an unused T is more expensive than the branching.

I think the generated code gets somewhat simplified once issue (1) is addressed.

I think it would be a mistake to just blindly copy the value of b when b.m_initialized is false, if for no other reason than doing so will lead to endless user complaints about compiler and valgrind warnings.  Also, invoking undefined behavior can result in the compiler doing very nasty and unexpected things, even in the absence of runtime issues from reading an "uninitialized" location.  Consider the possibility that the compiler can prove that the optional being copied from is uninitialized, and so can conclude that the read of its value is undefined behavior.  Probably the *best* one can hope for in such a situation is a compiler warning, and many far worse results are possible.

While I think this shouldn't be necessary from a theoretical standpoint, in a practical sense it might make the optimizer's job a little easier (and so increase the chances of getting the code you are looking for) to change the assign(optional) member functions that presently look something like

    if (is_initialized())
        if (rhs.is_initialized())
            assign_value(…)
        else destroy()
    else if (rhs.is_initialized())
        construct(…)

to instead be something like 

    if (rhs.is_initialized())
        if (is_initialized())
            assign_value(...)
        else construct(...)
    else destroy()

or

    if ( ! rhs.is_initialized())
        destroy()
    else if (is_initialized())
        assign_value(...)
    else construct(…)