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

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 
points.
> Additionally please consider giving feedback on the following general
> topics:
>
> - What is your evaluation of the design?
The design is sound but has some details that must be fixed before 
acceptation:

* (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 
specific clock.
* (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 
http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2013/n3711.pdf
*  (Serious)  Element queue::value_pop(); must be added to the queues 
and the interface should follow 
http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2013/n3533.html.
* (Minor) Barrier could include the completion function as Boost.Thread 
and http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2013/n3817.html.
* (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 
variations points

* 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 
Boost.Thread.
* (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
> problems?

(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.

darwin-4.7.1,clang-3.1,clang-3.2,darwin-4.7.2,darwin-4.8.0,darwin-4.8.1

I've run the tests.

* auto_ptr should be replaced.
* I've got this compile error
clang-darwin.compile.c++ 
../../../bin.v2/libs/fiber/test/test_futures_mt.test/clang-darwin-3.1xl/debug/link-static/threading-multi/test_futures_mt.o
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]
         return queue_op_status::success;
         ^      ~~~~~~~~~~~~~~~~~~~~~~~~
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?
>
>
Yes.

Best,
Vicente