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

Subject: Re: [boost] [optional] generates unnessesary code for trivial types
From: Hite, Christopher (Christopher.Hite_at_[hidden])
Date: 2012-01-26 10:22:36

Thanks for your feedback.

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

It would help, but I think won't get rid of all the branches.  Your refactoring might help more.

> 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.

Consider the completely legal code below:

struct cheap_optional_int{
        cheap_optional_int() : m_initialized() {} // don't init m_data

        bool m_initialized;
        int  m_data;
};

void assign_boost_cheap_optional_int(cheap_optional_int& a,cheap_optional_int& b){
        a=b;  // default impl
}

The compiler generates nothing but 32-bit moves from the source to the destination.  This is completely fine for valgrind.  It only complains if a branch based is taken based on uninitialized data.

00000000 <assign_boost_cheap_optional_int(cheap_optional_int&, cheap_optional_int&)>:
   0:   53                      push   %ebx
   1:   8b 44 24 0c             mov    0xc(%esp),%eax
   5:   8b 58 04                mov    0x4(%eax),%ebx
   8:   8b 08                   mov    (%eax),%ecx
   a:   8b 44 24 08             mov    0x8(%esp),%eax
   e:   89 08                   mov    %ecx,(%eax)
  10:   89 58 04                mov    %ebx,0x4(%eax)
  13:   5b                      pop    %ebx
  14:   c3                      ret

Sorry the assembler is so poorly formatted after it's mailed.

The cool thing is cheap_optional_int has_trivial_destructor and has_trivial_copy because we haven't overridden the defaults.

Unfotunately overriding the default ctor/dtor always breaks these, even if the code could be optimized out.  It may not even be possible for a compiler to solve.

Chris

_____________________________________________
From: Hite, Christopher
Sent: Wednesday, January 25, 2012 6:29 PM
To: 'boost_at_[hidden]'
Subject: [optional] generates unnessesary code for trivial types

When decompiling my code I noticed a bunch of unnessesary code caused by boost::optional.

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.

2) assignment also generates these problems:

void assign_boost_optional(optional_int& o){
        o=13;
}

Here there's a semantic issue: we have to decide to use the copy constructor or operator=.  This is also wasteful for POD types or any type which has_trivial_copy<>.

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.

4) has_trivial_destructor<T> should impl has_trivial_destructor< optional<T> > , but this is hard to implement without specialization of optional.

Checking has_trivial_destructor might take care of the complexity of optional<T&> since has_trivial_destructor< T& >.

I'd be willing to fix #1.  The other issues need some discussion.

Chris