Boost C++ Libraries: Ticket #1910: function::swap should avoid doing memory allocations

attachment set

niels_dekker — Sat, 10 May 2008 13:02:36 GMT

attachment → moving_function_swap.patch

cc set

niels_dekker — Fri, 29 Aug 2008 18:18:30 GMT

cc nd_mail_address_valid_until_2008-12-31@… added

Douglas Gregor — Fri, 05 Sep 2008 15:41:45 GMT

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.

status changed; resolution set

Douglas Gregor — Fri, 05 Sep 2008 15:43:24 GMT

status new → closed
resolution → fixed

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

niels_dekker — Fri, 05 Sep 2008 16:36:32 GMT

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 — Fri, 05 Sep 2008 23:23:58 GMT

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 — Sat, 06 Sep 2008 03:16:26 GMT

(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 — Sun, 07 Sep 2008 21:20:55 GMT

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

attachment set

niels_dekker — Thu, 11 Sep 2008 15:40:01 GMT

attachment → function_assignment.patch

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

niels_dekker — Thu, 11 Sep 2008 16:16:41 GMT

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