Re: [boost] Boost.Fiber review January 6-15

Subject: Re: [boost] Boost.Fiber review January 6-15
From: Agustín K-ballo Bergé (kaballo86_at_[hidden])
Date: 2014-01-15 20:56:30

On 06/01/2014 10:07 a.m., Nat Goodspeed wrote:> Hi all,
 > Please always state in your review whether you think the library 
should be
 > accepted as a Boost library!

My vote is to REJECT the library in its current state.

 > Additionally please consider giving feedback on the following general
 > topics:
 >
 > - What is your evaluation of the design?

The design is that of the C++11 thread API, so I'll only focus in the 
divergence points:

- The lack of a variadic constructor for `fiber` and variadic arguments 
for `async` makes it difficult to use the library correctly (even in the 
presence of C++14 lambdas). The semantic of those calls is that of a 
deferred call, which is difficult to achieve otherwise (note that `bind` 
doesn't help here).

- The interface can only accept a specific clock `time_point`. Correct 
use of Boost.Chrono is needed, the implementation can deal with a 
specific clock type internally.

- There is no support for deferred futures, which are incredibly useful 
for lazy evaluation.

- There are a number of minor issues with the interface (return types, 
parameters). These are easily fixable by looking at the standard.

- The safe-bool operator is a pointless divergence which only helps save 
a few keystrokes. I'm ok with them staying, but please prioritize the 
weak and missing points of the library first.

The overall impression is that the library leaves the boilerplate to 
users (bundling a deferred call, converting to a specific clock). Also, 
it's crucial to get the semantics for `fiber` constructors right which 
means being careful about certain details (like making sure they work 
with movable-only types), but those constructors are not there yet.

 > - What is your evaluation of the implementation?

I've only glanced at the implementation, and I have concerns about the 
quality of the code. For instance, the following pattern appears 
frequently and it's unsettling:

        #ifndef BOOST_NO_RVALUE_REFERENCES
                promise( promise && other)
                /* bunch of code... */
        #else
                promise( BOOST_RV_REF( promise) other)
                /* same code as above... */
        #endif

For completeness, the correct use of Boost.Move is:

     promise( BOOST_RV_REF( promise) other)
     /* code, just once... BOOST_RV_REF(promise) will be promise&& if 
there is rvalue-refs support */

Unfortunately I do not have enough time now to look into it in more 
detail, but incorrect use of Boost.Chrono and Boost.Move is not a 
promising start.

 > - What is your evaluation of the documentation?

The reference documentation looks OK. Some points are missing, but I've 
already raised those and Oliver agreed to take care of them.

 > - What is your evaluation of the potential usefulness of the library?

The potential usefulness of this library is huge. It goes beyond that of 
simply being an utility library for Boost.Asio. Fibers are a great 
replacement for threads for two key points: the ability to create 
thousands (or millions) of them, and performance (lighter than a 
thread). It was hinted that performance is not a goal of this library, 
as it is merely intended to provide a way to synchronize/coordinate 
coroutines with Boost.Asio. If that's the case, I'd suggest to move the 
library to the `asio` namespace and leave the `fiber` namespace open for 
a fiber library that targets a wider audience.

 > - Did you try to use the library? With what compiler? Did you have any
 > problems?

I did not.

 > - How much effort did you put into your evaluation? A glance? A quick
 > reading? In-depth study?

I've looked at the documentation, glanced over the implementation, and 
followed the debate on the mailing list.

 > - Are you knowledgeable about the problem domain?

I work in and with a fiber-based C++11 thread API implementation.

Regards,
-- 
Agustín K-ballo Bergé.-
http://talesofcpp.fusionfenix.com