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