Re: [boost] [context review]

Subject: Re: [boost] [context review]
From: Oliver Kowalke (oliver.kowalke_at_[hidden])
Date: 2011-03-25 03:34:53

Hello,

> The first issue is that the destructor of the context class will (at
> least by default) happily destroy the stack. This unfortunately will
> leak any resource owned by any object on the stack; This is not unlike
> destroying an allocator before having destroyed all allocated object.

This was already described in the docu.

> A stack can only be safely deallocated if either has been completely
> unwound or if it doesn't own any resources (note that is not enough to
> just destroy any stack allocated object). Context should either: a)
> provide an interface to unwind the stack (by automatically trowing a
> special exception for example), or b) detect whether the stack has not
> been unwound and assert in debug mode in the destructor.

Because boost.context should be a thin wrapper/building block the higher abstractions are responsible. If the execution context was yielded in the middle of its exec path it would require to jump back throw an exception (whic hmay consume a lot of CPU cycles).
Is this realy required? Couldn't it be reasonable for some implementations only to deallocate the stack?

> As it is, is not clear the level of abstraction this class wants to
> be. It looks like is trying, using RAII, to manage context creation
> and destruction safely, giving a false sense of security.

see above

> Context is neither copyable nor movable; I can understand the reason
> for making it non-copyable, but I see no reason why it shouldn't be
> movable. Without either capability it is hard to make context
> interoperate with existing code without forcing to dynamic allocation.

the reason not to support move-semantics relies on the support of ucontext.
ucontext_t can not be copied (as required by move-semantics) because the struct ucontext_t is is implementation defined and may vary from release to release. Many man pages of several OS definetly warn for copying ucontext_t (http://nixdoc.net/man-pages/HP-UX/man2/makecontext.2.html).

> Context is templated on both the context implementation and the stack.
>  The first problem is that the concept requirements for the Stack
> template arguments are not stated. 

I describe the requirements in the docu (implicit interface, move-semantics). I'm not sure which additional descriptions are required.

> Second, the stack type should not
> be part of the context type: the primary reason for specifying
> different stacks is for option of having the guard page: enabling this
> should not change the type of Context and it should possible to
> specify it at runtime. 

But enabling a guard page with mprotect() requires that the memory was allocated with mmap() othwerwise the behaviour is unspecified.

> The trampoline function can be defined in such a way that
> the custom deleter will be invoked after the stack is unwound.

I'm not so happy with automatically stack unwinding. It should be done by higher abstractions (see my notes above).

> Regarding the CtxT type, instead of this template parameter there
> should be a program wide default controllable at compile time by a
> macro; I saw elsethread that Oliver is already leaning on this idea.

yes - the second argument will be removed

> A good abstraction layer around the low
> level context, among other things, should support clean unwind of the
> stack (or detecting an not-unwound stack), generic functors (and not
> just function pointers) for type safety and custom deleters.

I think this is discussible: I think that detecting an unwound stack would be a good addition. But Ive some concerns regarding to the other points:
automatic stack unwinding, generic functors.
Thsi should handled/implemented by this low-layer lib. For instance generic functors would require an additional allocation and some higher abstractions might not use/want such generic functors but would have to pay for it.

> On the other hand the detail fcontext class seems a good, if maybe not
> complete, portable implementation of a low level ucontext-like API. I
> would be in favor of a simpler version of context which consists
> solely on the fcontext class (with a guarantee that this type is a
> POD), plus the make_context and swap_context functions. 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. A
> safe context class would just be one of the client of the API, other
> library authors could either use Context of the underlying fcontex
> directly depending on wether they need the speed or safety.

I was already requested to provide a C-lib regarding to fcontext.

> I think this basic API could still be tweaked [but my vote won't be
> conditional on that] to add extra functionality:
> 
>  * 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 contextes)

this is what boost.tasklet does

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

was my first thought too - but what happend if the user compiles it with NDEBUG?

> Make_fcontext requires a call to get_fcontex first, a-la ucontext:
> there is no reason for this, just merge get_context with make_context
> (and drop the unused set_fcontext).

this was required for the Windows fcontext implementation

> The posix ucontext wapper and the windows fiber wrapper should be made
> API compatible with fcontext.

could you explain it please - I'm uncertain what do you mean.

> Bot the i386 and amd64 asm implementation save and restore the fpu
> control word (the amd64 both x87 and SSE2 control words).

The SYSV AMD64 ABI requires that the SSE2 control and status word
and the x87 control word have to preserved across function calls.

> BTW, the current code is already inconsistent because it is saving the
> SSE2 control world on amd64 but not on i386.

I didn't read in the SYSV I386 ABI document about the fact that it requires to preserve SSE2 stuff.

> For posix systems lacking ucontext, fcontext can be implemented using
> the sigaltstack trick (this can actually be faster than ucontext if
> the non signal restoring _longjmp is used). For reference see:
> http://www.gnu.org/software/pth/rse-pmt.ps

I'll take a look at it - but AFAIK setjmp/longjmp are not required to preserve the stackframe. Maybe it will not work on all platforms.

> The fcontext class should be documented.

even if it is a implementation detail?
 

regards,
Oliver
-- 
GMX DSL Doppel-Flat ab 19,99 Euro/mtl.! Jetzt mit 
gratis Handy-Flat! http://portal.gmx.net/de/go/dsl