[Boost-announce] [boost] Boost.Context review result

Subject: [Boost-announce] [boost] Boost.Context review result
From: Vicente BOTET (vicente.botet_at_[hidden])
Date: 2011-05-14 04:58:34

Hi,

this was my first experience as review manager and it has been more difficult than I had expected. Apologies if I forget some important things or I misunderstood others. I hope that the major points have been taken in account.

Here it is the result of the Boost.Context review. The library is accepted provisionally and will need to address some mandatory points before a mini review (4/5 days).

There were not too much complete reviews (5) but globally all the reviewers voted for acceptation and two subject to some major redesign modifications. I hope I didn't miss somebody.

YES Gordon Woodhull
YES/Cnd Giovanni Piero Deretta 
YES Artyom
YES Nat Linden
YES Phil Endecott
YES/Cnd, Jeffrey Lee Hellrung, Jr

Thanks to all the review participants Gordon Woodhull, Giovanni Piero Deretta , Artyom, Nat Linden, Holger Grund, Phil Endecott, Jeffrey Lee Hellrung, Jr.. Special thanks to Giovanni Piero Deretta showing some major flaws in the global design and Holger Grund for showing a (possible) major bug on the Windows implementation. 

I know Oliver that you have already redesigned your library, but if there are points that you have not been addressed yet or have some difficulties addressing them, I will suggest you to work closely with Giovanni, he know well the domain and have a lot of good ideas, and exchange on this ML. You should follow up more closely the discussions during the mini review and even try to get feedback from the ML posting how you see your redesigned library. 

Point to be addressed before the mini review:

Design
1. Separation of the context class in two interface:
- A C-like interface that defines a POD struct that is memcpy-able and that can be implemented either using platform libraries, as ucontext, fibers or directly using assembler when a robust implementation is available. The context swap implementation don't need to save the signals as ucontext does. The following is just for illustrations purposes.

struct boost_context;
typedef void boost_context_fn_t(intptr_t);

extern "C" void boost_context_make(boost_context* ctx, boost_context_fn_t * f, intptr_t p, ...);
extern "C" void boost_context_swap(boost_context* from, boost_context const* to);
extern "C" intptr_t boost_context_destroy( boost_context* fc); 

boost_context_make is a merge of get_context with make_context + some fields assignations.

"A user will have lower expectations from such a low level API and at the same time will give more freedom for other people to implement on top of it higher abstractions as Boost.Corutines, Boost.Fibers ..."

- A non-copiable but movable safe context class on top of the low level C-like interface, that will provide modern and efficient C++ interface, that is usable by a user in a safe way. 

The safe context class would just be one of the client of the C-like interface, other library authors could either use the underlying C-like API directly or the class context depending on wether they need the speed or safety. 

* The context class will not be templated any more, and the use of type erasure will ensure that.
* The constructor will accept any C++ callable, function pointers, functors, ...
* At this high level interface, the function provided by the user could throw exceptions, and the library must wrap the user function and needs to store the exception raised and provide an interface to retrieve them.

2. The stack implementation must ensure that the stack is unwound before it is destroyed. A stack should only be safely deallocated if either has been completely unwound or if it doesn't own any resources.

A basic stack without protection seems mandatory.

For debug purposes it has been required to provide some stack usage functions that allow to know the max size used for example. 

Implementation
3. The assembler implementation on Windows must be disallowed if the ABI bug reported by Holger is confirmed. As far as Windows provides a fiber implementation that is as efficient as the assembler one, the assembler implementation is not mandatory and the user of the provided Windows fiber is a preferable choice.

4. Most of the 'throw' statements in the context class should actually be asserts. 

5. The provided stack should be used to store any context parameters as far as no major impediment is found during the implementation. This will avoid further allocations on the safe context class.

Documentation
6. The documentation is very minimalist and should be expanded with a discussion on context switching and use cases. Take care of all the grammar/spelling improvements suggested by the reviewers (It will be great if Jeffrey could review the documentation before the mini-review). Some user land examples should be provided (why not the yield example). 

7. The concept of (Movable) Stack must be clearly defined, as it is done in the standard, using expression requirements with semantic constraints.

8. The assembler implementation should be carefully documented, so others could maintain the code and add other implementations if needed.

9. Some performance measure should be added. Providing the number of cycles on each implemented platform would be nice.

Build system

10. The default build on a platform must work (It must be ABI compatible with the underlying platform) and take care of the specific features. Of course, the build needs to support cross compiling, so the user should be able to override the default values. 

Others points that are not required but that can make the library more usable or efficient:

1. The assembler implementations could be simplified if the context itself was represented as a single pointer to the top of the stack. This would save some instructions to move the source address from the stack return address to the context struct and back.

2. Instead of supporting a 'fc_link' a-la ucontext, have the callback function return a new context (it is void now). 'fc_link' is useful only on some limited cases (usually when you have an external scheduler managing your contexts), while the having the callback return the 'next continuation' has more applications (note that fc_link can be implemented easily in term of the continuation returning variant, but the reverse is harder). This will be very useful, for example, to implement some kind of 'exit_to' functionality (unwind the stack and restore another context), a key piece for safe context destruction.

3. Give to boost_context_swap the ability to exchange a parameter with the
other context:

boost_context_parm_t boost_context_swap( boost_context * ofc, boost_context const*
nfc, boost_context_parm_t parm)

where boost_context_parm_t is either

union boost_context_parm_t {
void * ptr;
int int_;
};

or simply intptr_t;

boost_context_swap saves the current context in ocf and restores the context ncf as usual; additionally, if ofc was 'captured' on a call to boost_context_swap, this call will return the value of parm on the original context. This extra parameter is equivalent to the second parameter of longjmp.

If ofc was created with boost_context_swap, 'parm' will be an extra parameter to the function.

Note that this can be potentially implemented with little or no overhead.

4. Add a variant of boost_context_swap that allows executing a function on the destination context (in this case 'parm' would be passed parameter to this function. This is mostly useful for injecting an exception on the destination context. This can be potentially implemented on top of the existing functionality, but would slow down every context switch; a possible efficient implementation would manipulate the destination stack to add a call to the function on top of it, so that the cost is paid only when required. 

5. Add a current context function (thread local).

Vicente
Review Manager

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost