Gdb 'catch throw' and boost::fibers::broken_promise when moving promises

[savinz]

I am seeing boost::fibers::broken_promise exceptions with gdb catch throw when
moving (std::move) boost::fibers::promise. This is due std::make_exception_ptr that gets
called when tmp object goes out of scope in the move assignment operator:

fiber/include/boost/fiber/future/promise.hpp

Lines 70 to 76 in e325cf7

	promise_base & operator=( promise_base && other) noexcept {
	if ( BOOST_LIKELY( this != & other) ) {
	promise_base tmp{ std::move( other) };
	swap( tmp);
	}
	return * this;
	}

Wouldn't be better to implement the move assignment the same way as the move constructor?

Test cases (test1 - gdb catch throw shows broken_promise, test2 - no throws):

#include <boost/fiber/all.hpp>
#include <iostream>
void test1() {
    boost::fibers::promise<int> p1, p2;
    auto f = p1.get_future();
    p2 = std::move(p1);
    p2.set_value(1);
    std::cout << f.get() << std::endl;
}
void test2() {
    boost::fibers::promise<int> p1;
    auto f = p1.get_future();
    auto p2{ std::move(p1) };
    p2.set_value(1);
    std::cout << f.get() << std::endl;
}

[olk]

promise implements move assignmentin terms of move constructor
test1() and test2() do not throw at my system (gcc-7.2 + clang-5.0.1/Linux) (even in gdb with catch throw)

[savinz]

Thanks, just tested with gcc-7.2 and looks like it does a way better job at optimizing the promise move
assignment operator than gcc-6.3.0. Gdb catch throw shows no throws. I did a quick benchmark:

gcc-6.3.0 (-O2)

-----------------------------------------------
Benchmark        Time           CPU Iterations
-----------------------------------------------
test1         1604 ns       1605 ns     435152
test2          111 ns        111 ns    6224944

gcc-7.2.0 (-O2)

-----------------------------------------------
Benchmark        Time           CPU Iterations
-----------------------------------------------
test1          332 ns        333 ns    2097035
test2          112 ns        112 ns    6193937

gcc-7.2.0 (-O2)
changed promise assignment to use future_ = std::move( other.future_) instead of swap

-----------------------------------------------
Benchmark        Time           CPU Iterations
-----------------------------------------------
test1          159 ns        159 ns    4370213
test2          111 ns        112 ns    6237259

[olk]

which version do you use?
promise_base & operator=( promise_base && other) is implemented with promise_base( promise_base && other) and the move contructor uses future_{ std::move( other.future_) }.

what happend with the exceptions you have reported?

[savinz]

I use the latest develop branch. The exceptions are really only visible with gdb catch throw due to
how the std::make_exception_ptr is implemented in gcc 6 - with try catch. No exceptions are
propagated into user program - sorry for not being clear. I stumbled on this while debugging
exceptions in my program. It seemed a bit costly beside making noise with gdb. Gcc 7 has a
different implementation of std::make_exception_ptr, it's faster and doesn't throw, so gdb doesn't
show it. Still promise_base & operator=( promise_base && other) implemented with tmp object
and swap results in future_->owner_destroyed() ... std::make_exception_ptr when tmp goes out
of scope. This is quite a performance difference - at least with gcc 6. In my program I am seeing a
12% improvement in speed if I change the move assignment operator to:

    promise_base & operator=( promise_base && other) noexcept {
        if ( BOOST_LIKELY( this != & other) ) {                                             
            obtained_ = other.obtained_; 
            future_ = std::move( other.future_);
            other.obtained_ = false; 
        }
        return * this;
    }

Or if I just avoid using it altogether and stick with moving promises using move constructor only.
Gcc 7 produces much faster code, but still it's a factor of 2x between move constructor and operator.

[olk]

the move assignment operator is mssing future_->owner_destroyed();

promise_base & operator=( promise_base && other) noexcept {
        if ( BOOST_LIKELY( this != & other) ) {                                             
            obtained_ = other.obtained_; 
            if ( future_) future_->owner_destroyed();
            future_ = std::move( other.future_);
            other.obtained_ = false; 
        }
        return * this;
    }

Otherwise a future constructed from a promise that gets move-assigned another shared-state will wait forever in future<>::get() or future<>::wait().

Suppose following example:

boost::fibers::promise<int> p1, p2;
auto f1 = p1.get_future();
auto f2 = p2.get_future();
p2 = std::move(p1);
p2.set_value(1);
std::cout << f1.get() << std::endl;
std::cout << f2.get() << std::endl;

f2.get() throws int the currrent version:
'The associated promise has been destructed prior to the associated state becoming ready.'

[savinz]

Ah, yes ... thanks for pointing this out.

I see there is the obtained_ flag and the promise_base get_future already has guards
if (obtained_)... and if (!future_ )..., so the future_->owner_destroyed() is only needed
in move assignment (and in the promise_base destructor) if obtained_ is true. Or am I
missing something?

[olk]

future_->owner_destroyed() is required if a shared-state is available
obtained is only need to prevent to generate a second future

[savinz]

I see future_->owner_destroyed() sets the broken_promise() exception if it has not been
marked ready. Since the future_ is created in promise_base constructor the owner_destroyed
will be called regardless of whether get_future() was called or not (the future_ is actually used -
shared). So if I do this:

    boost::fibers::promise<int> p1, p2;
    auto f = p1.get_future();
    p2 = std::move(p1);

I face the overhead of temporary future_ object and more importantly std::make_exception_ptr
which is significant under gcc 6 (I yet check clang or msvc). The move constructor doesn't
have this overhead, this is really what I wanted to point out.

[olk]

Maybe a modification of ~promise_base() could help:

    ~promise_base() {
        if ( future_ && obtained_) {
            future_->owner_destroyed();
        }
    }

shared_state::owner_destroyed() is called when a shared-state was initialized and a future<> was created from it. could yout test it/measure the timing?

[savinz]

gcc-6.3.0 (-O2)

-----------------------------------------------
Benchmark        Time           CPU Iterations
-----------------------------------------------
test1          160 ns        160 ns    4362747
test2          111 ns        111 ns    6207349

gcc-7.2.0 (-O2)

-----------------------------------------------
Benchmark        Time           CPU Iterations
-----------------------------------------------
test1          158 ns        158 ns    4396806
test2          112 ns        112 ns    6195495

With this change promise move assignment is ~10x faster with gcc 6 and ~2x faster with gcc 7 :-)

[olk]

commit pushed, ty