function::swap should avoid doing memory allocations

[niels_dekker]

This ticket was triggered by a posting from Arvid Norberg, ​boost::function<>::swap() can throw. He was surprised to see that boost::function::swap can throw an exception, in case of a memory allocation failure. And he also remarked that TR1's function::swap is specified not to throw.

The current function::swap (​function_template.hpp, from trunk revision 43884) is implemented by doing three copy operations:

    void swap(BOOST_FUNCTION_FUNCTION& other)
    {
      if (&other == this)
        return;

      BOOST_FUNCTION_FUNCTION tmp = *this;
      *this = other;
      other = tmp;
    }

I think that a major cause of exceptions can be taken away, by having function::swap implemented by means of moving, instead of copying:

    void swap(BOOST_FUNCTION_FUNCTION& other)
    {
      if (&other == this)
        return;

      BOOST_FUNCTION_FUNCTION tmp;
      tmp.move_assign(*this);
      this->move_assign(other);
      other.move_assign(tmp);
    }

This new boost::function::move_assign member function would in most cases just copy the function object held by its argument to *this, just like boost::function's copy assignment operator does. But if the argument would hold its function object within a buffer allocated on the heap, move_assign would pass the buffer to *this, and set the argument's buffer pointer to NULL.

The patch has boost::function::move_assign added to function_template.hpp. Out of modesty, I declared move_assign as a private member function, but feel free to make it public. Moreover, it could be changed to move from *this to its argument (and renamed, e.g., to move_to), so that it would support moving a temporary (rvalue). But I think that's beyond the scope of this ticket...

Implementation details

The attached patch has a boolean argument added to all manage functions, telling whether or not the buffer should be moved. Thereby, the implementation of move_assign is quite similar to the existing assign_to_own, having the same call to f.vtable->manager, but passing true as last argument.

This boolean argument move_buffer is only relevant to the specific manager overload for function objects that require heap allocation, which does in the patched version of function_base.hpp:

  if (move_buffer)
  {
    out_buffer.obj_ptr = in_buffer.obj_ptr;
    in_buffer.obj_ptr = 0;
  }

I made obj_ptr mutable, to work around the fact that in_buffer is a const reference. Instead, I guess a const_cast could be used, but I didn't have the guts to do so. ;-)

[Douglas Gregor]

This patch should give a significant performance improvement for swap(). Note, however, that it does not provide the nothrow guarantee for swap() due to the small-object optimization: small objects are still copied by move_assign, so their constructors could throw. We may or may not want a no-throw swap for Boost.Function, regardless of what std::(tr1::)function say; that we can handle on-list.

I'm applying a variant of this patch to trunk now, and will close the ticket then.

[Douglas Gregor]

(In [48615]) Improve the performance of Boost.Function's swap. Thanks to Niels Dekker for the original patch. Fixes #1910

[niels_dekker]

Thank you, Doug! I'm not sure if we should sacrifice the small-object optimization to get a strictly nothrow swap. For the time being, the function::swap performance improvement you've just committed looks like a reasonable compromise to me.

[niels_dekker]

Replying to dgregor:

(In [48615]) Improve the performance of Boost.Function's swap.

Sorry, I'm not yet entirely sure about your fix of ​function_base.hpp, as it says in manage_small:

  if (op == move_functor_tag) {  
     reinterpret_cast<functor_type*>(&in_buffer.data)->~Functor();  
  }

As a consequence, it seems to me that the functor may be destructed too many times. The following BOOST_CHECK fails on my computer:

    static unsigned construction_count;
    static unsigned destruction_count;

    struct MyFunctor {
        MyFunctor() { ++construction_count; }
        MyFunctor(const MyFunctor &) { ++construction_count; }
        ~MyFunctor() { ++destruction_count; }
        int operator()() { return 0; }
    };


int test_main(int, char* [])
{
    {
      boost::function0<int> f;
      boost::function0<int> g;
      f = MyFunctor();
      g = MyFunctor();
      f.swap(g);
    }
    // MyFunctor objects should be constructed at least
    // as many times as they are destructed.
    BOOST_CHECK(construction_count >= destruction_count);
}  

[Douglas Gregor]

(In [48627]) Fix double-destruction problem with small function objects and swap(), and try to work around a GCC 4.2 issue. See #1910 for comments about the former problem from Niels Dekker.

[niels_dekker]

Now that boost::function internally has a move_assign function, I think that the performance of its assignment operators can also be improved significantly, by calling move_assign, instead of swap. Please have a look at the attached function_assignment.patch

[niels_dekker]

boost::function assignment operators calling move_assign. (Update: the patch now has a by-value argument for the copy assignment operator.)

[niels_dekker]

Please note: I made a separate ticket regarding my previous comment (including function_assignment.patch): Ticket #2319 - function::operator= should "move", copy assignment should have by-value argument