Boost logo

Boost :

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:

                promise( promise && other)
                /* bunch of code... */
                promise( BOOST_RV_REF( promise) other)
                /* same code as above... */

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.


Agustín K-ballo Bergé.-

Boost list run by bdawes at, gregod at, cpdaniel at, john at