Re: [boost] [context] mini-review comments

Subject: Re: [boost] [context] mini-review comments
From: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2012-01-13 03:25:35

On Thu, Jan 12, 2012 at 11:14 PM, Oliver Kowalke <oliver.kowalke_at_[hidden]>wrote:

> Hello Jeff,
>
> thx for your review
>
>
> > How do you store the function and function arguments passed to the
> > context constructor?
>
> the function arguments are bound with the function to a functor using
> boost::bind(). This functor is stored inside context_object class (derived
> from context_base (both in sub-namespace detail).
>

Ummm, okay, which begs the question: where does this context_object object
live?

> > I couldn't find it in the documentation, and whatever the answer, seems
> > like something that should be mentioned.
>
> Hmm - it is an implementation detail. Should I document it in the
> 'Rational' section?
>

Well, it seems like you need to dynamically allocate *something*, and
whether this is through operator new, the stack allocator (probably
preferable...?), or something else, I think it should be documented

[...]

> > Given that void pointers are used to communicate between contexts,
> > I almost would've expected the context constructor to simply take
> > a void (*)( void*) argument and context::start to take a void*
> > argument which would be passed to the function pointer.
> > Can you elaborate on the rationale for the low-level interface for
> > communicating between contexts while still providing a relatively
> > high level interface for specifying the context function?
>
> With context::start you enter the context-function. The arguemnts for the
> context-fn are taken by the ctor of context.
> context::resume and context::suspend have a void* arg because it is used
> to transfer data between context jumps.
>

Okay. In other words: you can type-erase the context function quite easily,
since you use the pimpl idiom anyway. On the other hand, type-erasing the
data transferred between contexts would necessitate, e.g., boost::any, and
hence a potentially unnecessary dynamic allocation and (maybe) dynamic
dispatch.

context ctx;
> void f( int i) {
>  ...
>  char * x = "abc";
>  void * vp = ctx.suspend( x);
>  double * d = static_cast< double >( vp);
>

Of course, you mean double* here, right? (If this is an actual example from
the library, be sure to change it!)

>  ...
> }
> ctx = context( f, 1);
> void * vp = ctx.start();
> char * x = static_cast< char * >( vp);
> double d = 2.01;
> ctx.resume( & d);
>
> the cotnext-fn 'f()' is passed to contexts ctor with the integer arg '1'.
> ctx.start() starts the context == enter f() with argument i == 1.
> ctx.suspend() suspends execution of context-fn f() so we return from
> ctx.start() and transfers x == 'abc' as return arg of ctx.start().
> ctx.resume() re-enters context-fn f() and the return value of
> ctx.suspend() is a pointer to the double withvalue 2.01.
>
>
> > The documentation for the stack allocator concept doesn't completely
> > define the semantics of the size parameter of
> > stack_allocator::allocate/deallocate. I presume it is the stack size in
> > bytes, but that should be explicit in the documentation. And I presume
> > allocate is only required to return a pointer to *at least* size bytes of
> > free memory.
>
> yes
>
> > Also, you don't mention alignment requirements; what are
> > they?
>
> the stack will be aligned automatically by boost.context - the user has
> not to worry about it.
>

I'm not sure who you mean by the "user". Just to be clear: I'm asking, what
are the alignment requirements of the void* returned by allocate within the
stack allocator concept? For those who want to create their own custom
stack allocator.

> I'm somewhat surprised default_stacksize() always returns 256*1024. I
> > think
> > it might be safer to make this implementation-defined, but some minimum
> > size (as with everything else in C++...).
>
> which value should be choosen for default_stacksize()? The operating
> system doesn't define it (but minimum/maximum stacksize is defined) - it is
> a heuristic value.
>

Let's be clear, I'm not challenging your choice of the default stack size;
of course it's heuristic. What I'm pointing out is what I perceive to be an
over-specification of the interface. I don't think it's a good idea to
guarantee (as much as one is able to, anyway) a one-size-fits-all default
stack size for all present and future platforms for all time and across all
dimensions and parallel universes. For the same reason that C++ doesn't
define an int as a specific number of bits. It seems preferable to make it
"implementation defined" and, perhaps, note its value on some common
platforms. Does that make sense?

That aside, can you provide some rationale for the specific choice of 256
kB? It seems rather...arbitrary, that's all. Indeed, I would've expected
the default stack size to be somehow related to the platform word size
(sizeof( void* ), maybe?).

> Just a nitpick on member variable names.
> > boost_fcontext_stack::ss_base/ss_limit: what does "ss" mean?
>
> names base and limit are inspired by MS Windows Fiber API.
>

Hmmm...okay. That doesn't exactly answer my question, though :/

- Jeff