Hello, This is my formal review.
Please always state in your review, whether you think the library should be accepted as a Boost library!
Yes, it should be a part of boost. (the alternative version with improved f/u-context support) However following issues should be addressed ASAP: 1. Documentation must be improved with much better description, rationale, examples and design notes and description of member functions, stack unwinding in case of context destruction and so on. See notes below. 2. Current build is too verbose. All relevant options should be configured automatically by default without user specifying them. I understand that it is limitation of BBv2 but it MUST be addressed in the official release. Now Full Review: ----------------
What is your evaluation of the design?
I had issues with Design of fcontext/ucontext but it was addressed by Oliver in very fast and I think correct way so I have no more problems with it. Also I have important concerns about verbosity of build system use. I've discussed this before. BBv2 improvements required.
What is your evaluation of the implementation?
The heaviest part of implementation is in assembly so I can't say too much. C++ code looks fine. Current issues: Extern "C" required in context_ucontext.hpp typedef void ( * stack_fn)(); should be extern "C" you need to pass pointers to makecontext as extern "C" function. Asm implementations: Add "namespace" names like boost_get_context instead of get_context. It would prevent names collision and in general allows tools like BCP to rename namespace to use two versions of boost library.
What is your evaluation of the documentation?
This is the weakest point of this library. The documentation is not sufficient. What do I miss: a) Context destruction and unwinding. When I tried to implement yield like iterator I had problem with unwinding the stack of auxiliary context when the program does not get execution. I should add some throwing in yield and some flags to make sure that stack is unwind in destruction of iterator. The behavior should be documented and examples should be provided. I explain more: Consider implementation like ... void iterate() { object foo; // may be not destroyed for(;;) { global_value = local_value++; if(local_value > 100000) return; else my_context.jump_to(other_context); } } ... int operator++() { other_context.jump(my_context); return global_value; } ... iterator it; while(it++ < 10) ; ... Now what happens when other_context does not return execution to my_context when gloabl_value becomes 10. Then foo object is not get destroyed. So I need explicity switch to my_context and throw something to make stack unwind procedure work and foo is not got destroyed. Such things should be documented and example how to use it should be provided (i.e. what is correct way to unwind the stack). b) Design rationale is missing c) I'd like to see comparison of features ucontext and fcontext provide. A table of limitations/implementations/platforms like Limitations\Methond ucontext fcontext Signals ... Performance ... d) Description of member functions of boost::context is too brief. e) Implementation details description should be given. f) Better examples should be provided, I've attached few programs I used to test Boost.Context that implements yield like iterator and using context with Boost.Asio in thread like run and such examples should be given. If you want to use the code of the exapmples, consider them under BSL. g) The option context-impl is removed in alternative version and mistakenly was left in the docs.
What is your evaluation of the potential usefulness of the library?
It is useful. I implemented a simple client server using Boost.Asio and Boost.Context that switch contexts on async requests and it was quite easy and straight-forward and brings very interesting paradigm to C++. I also had implemented a yielding iterator and it is was very nice, I like a fact that I have such concepts in C++.
Did you try to use the library? With what compiler? Did you have any problems?
I tested in on Linux x86_64 with gcc-4.4 I had problems that executable crashes using shared object libboost_concept.so and fcontext (when it is not compiled with -fPIC which is correct!) The author confirmed the bug and fixed it, no more problems seen.
How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I mostly tried to implement several tools like iterator and use it with Asio. I run some benchmarks and was quite impressed with fcontext performance. I spend about 6 hours on reviewing this library I implemented a simple client server using Asio and Context where in one case one side was reading and other writing and in other case it was echo server and client benchmark results were quite nice: Reader/Writeer Asio ------------------- Real-Threads, 2 CPU 0.95s Real-Threads, 1 CPU 1.53s ucontext 3.58s fcontext 2.90s Asio-Async 2.84s Echo Asio ---------- Real-Threads, 2 CPU 12.30s Real-Threads, 1 CPU 6.58s ucontext 7.24s fcontext 5.23s Asio-Async 5.24s So I think it is very good base to start from.
Are you knowledgeable about the problem domain?
I hadn't used ucontext and fibers before, but I'm familiar with event driven programming and threading. I have large experience and asynchronous programming and implemented once threading support for MS DOS and for Z80. Additional Unrelated Note: -------------------------- Would you consider to create C version of Boost.Context or create C API. It is very useful to have fast setcontext/makecontext operations in C because it may be useful for too many systems where C API is must easier to integrate then C++. Best Regards, Artyom Beilis