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

Subject: Re: [boost] Boost.Fiber review January 6-15
From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2014-01-10 02:50:53

>
> Please always state in your review whether you think the library should be
> accepted as a Boost library!
>

Library should be accepted, but some modification must be applied (see
below).

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

Part that mimics the Boost.Thread interface is good. Missing functions and
minor updates can be easily added even after the review.

Scheduler does not look good. Class "algorithm" looks overcomplicated: too
many methods, classes from detail namespace are used in function
signatures, looks like some methods duplicate each other (yield, wait).

I'd recommend to hide "algorithm" from user for nearest releases in detail
namespace, refactor it and only after that - show it to user. Maybe an
additional mini review will be required for algorithm.

> - What is your evaluation of the implementation?
>

I'm slightly confused by the fact that fiber_base use many dynamic memory
allocations (it contains std::vector, std::map) and virtual functions.
There must be some way to optimize away that, otherwise fibers may be much
slower than threads.

Heap allocations also exist in mutex class. I dislike the `waiting_.
push_back( n);` inside the spinlocked sections. Some intrusive container
must be used instead of std::deque.

There is a copy-pasted code in mutex.cpp. `try_lock` and `lock` member
functions have very different implementations, that's suspicious.

Back to the fiber_base. There's too many atomics and locks in it. As I
understand, the default use-case does not include thread migrations so
there is no need to put all the synchronizations inside the fiber_base.
round_robin_ws and mutexes work with multithreading, ideologically it's the
more correct place for synchronizations. Let fiber_base be thread unsafe,
and schedulers and mutexes take care about the serializing access to the
fiber_base. Otherwise singlethreaded users will loose performance.

> - What is your evaluation of the documentation?
>

Please add "Examples" section. Paul Bristow already mentioned a QuickBook's
ability to import source files using [@../../example/my_example.cpp] syntax.

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

I have a few complicated libraries that will benefit from fibers. However
short real-life usecase examples will be good to get the idea of fibers to
new library users.

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

There were no problems with GCC-4.7 and clang. Many warnings were rised
during compilation, but most part of the warnings belong to the DateTime
library.

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

I've tried a few examples, made a bunch of experiments with asio. Spend few
hours looking through the source code.

> - Are you knowledgeable about the problem domain?
>

Not a pro with fibers, but have good experience with threads.

-- 
Best regards,
Antony Polukhin