Subject: Re: [boost] Boost.Fiber review January 6-15
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2014-01-13 13:20:18
Le 06/01/14 14:07, Nat Goodspeed a écrit :
> Hi all,
> The review of Boost.Fiber by Oliver Kowalke begins today, Monday January
> 6th, and closes Wednesday January 15th.
Hi, here it is my review.
I would like to have more time to review the implementation, but as
there are already serious concerns respect to the design and
documentation, I will do it if there is a new review.
> Please always state in your review whether you think the library should be
> accepted as a Boost library!
IMO the library needs some serious modifications before been include
into Boost. So, for the time been, my vote is no, the library is not yet
ready. I'm sure that Oliver could take care of most of the major points
and that the library would be accepted after taking in account these
> Additionally please consider giving feedback on the following general
> - What is your evaluation of the design?
The design is sound but has some details that must be fixed before
* (Showstopper) The interface must at least follows the interface of the
standard thread library (C++11) and if there are some limitations, they
must be explicitly documented.
* (Serious) Any difference respect Boost.Thread must also be documented
and the rationale explained.
* (Minor) priority and thread_affinity should be part of the fiber
attributes as well as the stack and allocator. This will help maintain
the interface similar to the Boost.Thread one.
* (Minor) The
void thread_affinity( bool req) noexcept;
could be named set_thread_affinity to follow the standard way.
* (Minor) I suggest to hide the algorithm functionality and introduce it once you have a fiber_pool proposal that uses work stealing.
* I you insists in providing it I suggest:
*(Minor) set_scheduling_algorithm should return the old scheduling algorithm.
algorithm*set_scheduling_algorithm( algorithm *);
and the function should be on this_thread namespace.
* The migration interface
*(Serious) The steal_from and migrate_to functions could be grouped
on a exchange function that would do the steal and the migration at
once. This allows intrusive implementations that could avoid to
delete/new. If the algorithm doesn't supports fiber migration a
exception could be thrown.
* (Minor) An alternative could be to use an enum for the algorithm
class and let the library manage internally with the scheduler.
* (Minor) The safe_bool idiom should be replaced by a explicit operator
bool on C++11 compilers.
* (Showstopper) The time related function should not be limited to a
* (Minor) The fiber_group should be removed as the design it is based
based on the new C++11 move-semantic feature. Maybe it could be replaced
by one based on
* (Serious) Element queue::value_pop(); must be added to the queues
and the interface should follow
* (Minor) Barrier could include the completion function as Boost.Thread
* (Serious) Interruptible and non-interruptible threads must be separated.
* (Serious) Intra-thread fibers should be provided (fibers that
synchronize only with fibers on the same thread)
* (Serious) future<>::then() and all the associated features must be added. http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2013/n3784.pdf
> - What is your evaluation of the implementation?
I have take a quick reading at.
I would need much more time to have a reasonable evaluation, which I
don't have :(
As others I would like to see performance test on the following
* Single/Multi-threaded fibers
* Interruptible/NonInterruptible fibers
* Stealing/No Stealing schedulers
- What is your evaluation of the documentation?
(Serious) The documentation is minimal and should be improved.
* (Minor) The link on Boost.Coroutine fails as well as the link to
* (Serious) It must be clarified that the exceptions thrown by a fiber
function are caught by the fiber library and the program is terminated.
* (Minor) some examples showing how the affinity can be changed by the
owned of the thread and the thread itself would help.
* (Minor) Please don't document get/set functions as thread_affinity altogether.
* (Serious) The documentation must clarify the scope of the scheduler (thread specific).
* (Serious) The documentation must clarify how portable is the priority if it is specific of the scheduler.
* (Serious) You mustn't document the interface of the algorithm class that the user can not use.
* (Showstopper) async() documentation must be added and be compatible with the C++ standard.
* (Serious) A section on how to install and how to run the test and examples would help a lot to users that want to try the library.
It is not clear that the user must install fiber inside a Boost repository.
It is not clear in the documentation that the library is not header only and that the user needs to build it and link with it.
* (Serious) The boost/fiber/asio files are not documented. Are these a detail of the implementation?
- What is your evaluation of the potential usefulness of the library?
Very useful if the promised performances (C10k problem) are there. A
performance benchmark must be added.
> - Did you try to use the library? With what compiler? Did you have any
(Serious) Yes and a section on how to install the library and how to run the tests would help me.
on MacOs with the following compilers with c++98 and c++11 modes.
I've run the tests.
* auto_ptr should be replaced.
* I've got this compile error
In file included from test_futures_mt.cpp:13:
In file included from ../../../boost/fiber/all.hpp:17:
../../../boost/fiber/bounded_queue.hpp:570:29: error: void function
'push' should not return a value [-Wreturn-type]
if ( is_closed_() ) return queue_op_status::closed;
../../../boost/fiber/bounded_queue.hpp:580:9: error: void function
'push' should not return a value [-Wreturn-type]
2 errors generated.
The tests test_then and test_wait_for don't compile
> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
In-depth study of the documentation.
> - Are you knowledgeable about the problem domain?
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk