Re: [boost] Boost.Context review result

Subject: Re: [boost] Boost.Context review result
From: Vicente BOTET (vicente.botet_at_[hidden])
Date: 2011-05-15 05:10:09

Please, don't top post.

> Am 14.05.2011 23:29, schrieb Vicente Botet:
> > Oliver Kowalke wrote:
> >> Hi,
> >>
> >> thank you Vicente for your job as review manager.
> >>
> >> I've reworked the library according to the suggestions of the review -
> >> but not all requests could be fulfilled.
> >> The new version (boost.context-0.7.0.zip) is published on boost vault
> >> and the docu can be read here :
> >> http://ok73.ok.funpic.de/boost/libs/context/doc/html/ .
> >>
> >
> > Oliver please, be free to report which points can not be fulfilled.

> Message du 15/05/11 00:00
> De : "Oliver Kowalke" 
> A : boost_at_[hidden]
> Copie à : 
> Objet : Re: [boost] Boost.Context review result
> 
> * The context class will not be templated any more, and the use of type erasure will ensure that.
> 
> context must be templated -> reason is that hte stack must be owned by context -> see rational section in documentation

I have added the rationale

"Stack as template argument

A context must own stack because it must control the life time of the stack, for instance if the destructor of context has to unwind the stack the nthe stack must be still valid. For reusage of the stack it can be released - unwinding will not be done and the context can be used anymore. In orde to release the stack its type must be known by context therefore context takes the stack type as template argument. Usualy an application uses instances of _contexts_ with the same stack type instead of mixing contexts with with different stack types." 

I don't think that this is a valid rationale. Type erasure should allow you to do what you need. See the comments from Giovanii on how to do it.

> * 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.
> 
> this has to be done by higher level libs - you could use futures for instance (as boost.strand does)

After the split on a high level context class and a C-like API, the class context a high level class that should provide some user friendly features.

> 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.
> 
> asm impl for windows should be ok - all boost_fcontext_t functions are leaf-functions using tail calls - concerns from Holger do apply only to non-leaf functions

If the bug is not confirmed, then your library would satisfy this point. I will just heard what Holger has to say.

> 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.
> 
> This coding style can not be used toghether with move operations - explanation is in rational section in boost.context docu.

I think you have not understood how Giovanii proposed to do that, but maybe I'm wrong.

I have added your rationale so your point of view is more clear. 

"
Pimpl idiom

In order to support move semantics and context switching operations it was required to separate both features in separat classes via pimpl idiom. A nice featre is that a context has the size of a pointer, so it fits into a register.

With the current design the constructor accepts functors and its arguments+ similiar to thread.

What if pimpl idom wouldn't be used? Because stack unwinding is required a reference to control structures (like fcontext_t) is required inside the trampoline function (on top of stack). If the context object was moved the refernces to the control structures on the top of the stack pointing to invalid control structures.

Why not putting all the context control structures on top of the stack? If the control structures reside on top of the stack (inside the function body of the trampoline function) the context control structures would be destructed if the trampoline function returns (that happens if the functor passed to the constructor of context returns == the context is complete). If the control structures are destructed informations about the state of the context are lost. 
"

> 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.
> 
> boost.build is the default build system for boost libs - it has its limitations. those limitations are that the required boost.build properties as 'architecture', 'instruction-set' and 'address-model' are optional instead default. That means that optional properties are not set by the boost.build system. The user hast to specify those properties. In the documentation I wrote a section listing for all supported platforms the required property values.
> The mentioned properties are required for alternative selection in the Jamfile in order to select the correct assembler file.
> Vladimir told me that he is already working on boost.build so that 'architecture', 'instruction-set' and 'address-model' become default properties.

I have just a question: How people running the regression test will do to build your library and run your tests?

As stated on the review summary the following points are not required.

> > 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.
> 
> As described above this doesn't work - please take a look in the docu.
> 
> 
> 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),
> 
> boost.context was made to be used by my other lib boost.strand which provides a scheduler for such context objects - I think it is not a limited case.

Best,
Vicente