On 6 Jan 2014 at 8:07, Nat Goodspeed wrote:
Please always state in your review whether you think the library should be accepted as a Boost library!
Currently I am undecided. My biggest issue is in the (lack of) documentation. Without very significant improvements in the docs I'd have to recommend no right now.
Additionally please consider giving feedback on the following general topics:
- What is your evaluation of the design?
The design overall is fine. I agree with the decision to replicate std::thread closely, including all the support classes. My only qualm with the design really is I wish there were combined fibre/thread primitives i.e. say a future implementation which copes with both fibers and threads. I appreciate it probably wouldn't be particularly performant, but it sure would ease partially converting existing threaded code over to fibers, which is probably a very large majority use case. I accept this can be labelled as a future feature, and shouldn't impede entering Boost now.
- What is your evaluation of the implementation?
The quality of the implementation is generally excellent. My only issue is that the Boost.Thread mirror classes do not fully mirror those in Boost.Thread. For example, where is future::get_exception_ptr()? I think mirroring Boost.Thread rather than std::thread is wise for better porting and future proofing. I'd recommend finishing matching Boost.Thread before entering Boost. Another suggestion is that the spinlock implementation add memory transactions support. You can find a suitable spinlock implementation in AFIO at https://github.com/BoostGSoC/boost.afio/blob/master/boost/afio/detail/ MemoryTransactions.hpp.
- What is your evaluation of the documentation?
My biggest concerns with admitting this library now are with the documentation: 1. There is no formal reference section. What bits of reference section there is does not contain reference documentation for all the useful classes (e.g. the ASIO support). 2. I see no real world benchmarks. How are people supposed to know if adopting fibers is worthwhile without some real world benchmarks? I particularly want to know about time and space scalability as the number of execution contexts rises to 10,000 and beyond. 3. I see no listing of compatible architectures, compilers, etc. I want to see a list of test targets, ones regularly verified as definitely working. I also want to see a list of minimum necessary compiler versions. 4. I deeply dislike the documentation simply stating "Synchronization between a fiber running on one thread and a fiber running on a different thread is an advanced topic.". No it is not advanced, it's *exactly* what I need to do. So tell me a great deal more about it! 5. Can I transport fibers across schedulers/threads? swap() suggests I might be able to, but isn't clear. If not, why not? 6. What is the thread safety of fiber threading primitives? e.g. if a fibre waits on a fibre future on thread A, can thread B signal that fibre future safely? If not, what will it take to make this work, because again this is something I will badly need in AFIO. 7. I want to see worked tutorial examples in the documentation e.g. step me through implementing a fibre pool, and then show me how to implement a M:N threading solution. That sort of thing, because this is another majority use case. 8. There are quite a few useful code examples in the distribution in the examples directory not mentioned in the formal docs (which is bad, because people don't realise they are there), but they don't explain to me why and how they work. I find the ASIO examples particularly confusing, and I don't understand without delving into the implementation code why they work which is a documentation failure. This is bad, and it needs fixing. 9. It would help a lot to understand missing features if there were a Design Rationale page explaining how and why the library looks the way it does.
- What is your evaluation of the potential usefulness of the library?
It's very useful, and I intend to add fiber support to proposed Boost.AFIO.
- Did you try to use the library? With what compiler? Did you have any problems?
Not yet. AFIO needs to be modified first (targeted thread_source support).
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A few hours as I decided exactly how AFIO will add Fiber support. I reviewed both the source distribution and the docs.
- Are you knowledgeable about the problem domain?
Yes. I wrote one of these myself in assembler in the 1990s. Niall -- Currently unemployed and looking for work. Work Portfolio: http://careers.stackoverflow.com/nialldouglas/