[context] mini-review comments
[Late, I know] [I'm going off http://ok73.ok.funpic.de/boost/libs/context/doc/html/index.html] [Apologies if these are just repeating previous comments!] How do you store the function and function arguments passed to the context constructor? Are they bundled into memory allocated by the stack allocator? I couldn't find it in the documentation, and whatever the answer, seems like something that should be mentioned. "The maximum number of arguments of *context-function* is defined by BOOST_CONTEXT_ARITY (default is 10) and might be changed." Hopefully you mean, more specifically, that BOOST_CONTEXT_ARITY might be *increased*. Also, consider using variadic templates if available? Great examples! Would it be possible to also show the expected output, just so the reader can verify that he or she understands what should be happening? 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? 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. Also, you don't mention alignment requirements; what are they? 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++...). Also, strictly speaking, the documentation is incorrect. It reads "Defines a default stack size of 256 kB", but defalt_stacksize() doesn't define anything, it just returns a number. And this description is inconsistent with the remaining function descriptions. I would do: "Returns the default stack size, which is implementation-defined but at least 256 kB." In "Low level API (boost_fcontext_t)", s/stated/started/. Also, what's the magic number 262144? Oh, I see, 262144 = 256 * 1024...maybe the latter expression would be more intuitive? Again, I can guess what the output of the example would be, but would be helpful to have the expected output handy. Just a nitpick on member variable names. boost_fcontext_stack::ss_base/ss_limit: what does "ss" mean? boost_fcontext::fc_stack/fc_link: I presume the prefix "fc" is short for "fcontext", in which case, isn't it redundant? Normally I wouldn't be picky about member variable names but these are precisely the documented API. In "Rationale", you give "[o]ther low level APIs and their caveats"; I'm guessing these are, specifically, alternatives to boost_fcontext_t but are *deficient* in some respect. It might clear things up if you make that explicit, and further group "setjump()/longjmp()", "ucontext_t", and "Windows fibers" into a common heading. Overall, I think the documentation and interface are good. I didn't look at the implementation, but I did see that someone else did, so...I'm not too worried about a correct implementation. As long as there's a full suite of tests. I also didn't try to use the library, but now I'll have to find an excuse :) These are all relatively minor comments, and although I hope Oliver will address them, it won't hold up my approval of Boost.Context. - Jeff
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).
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?
"The maximum number of arguments of *context-function* is defined by BOOST_CONTEXT_ARITY (default is 10) and might be changed." Hopefully you mean, more specifically, that BOOST_CONTEXT_ARITY might be *increased*.
that's the intention
Also, consider using variadic templates if available?
of course
Great examples! Would it be possible to also show the expected output, just so the reader can verify that he or she understands what should be happening?
yes
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. context ctx; void f( int i) { ... char * x = "abc"; void * vp = ctx.suspend( x); double * d = static_cast< double >( vp); ... } 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 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.
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.
In "Rationale", you give "[o]ther low level APIs and their caveats"; I'm guessing these are, specifically, alternatives to boost_fcontext_t but are *deficient* in some respect. It might clear things up if you make that explicit, and further group "setjump()/longjmp()", "ucontext_t", and "Windows fibers" into a common heading.
OK regards, Oliver -- Empfehlen Sie GMX DSL Ihren Freunden und Bekannten und wir belohnen Sie mit bis zu 50,- Euro! https://freundschaftswerbung.gmx.de
On Thu, Jan 12, 2012 at 11:14 PM, Oliver Kowalke <oliver.kowalke@gmx.de>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
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'm using pimpl as member in context class (see below)
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
the pattern (using pimpl; type erasure of fn() and args) is also used by boost::thread and boost.threads docu does not document it because it is a implementation detail.
Okay. In other words: you can type-erase the context function quite easily, since you use the pimpl idiom anyway.
yes
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.
you refering to context::supend()/context:.resume()? boost::any is not used. the lib does only transfer the pointer to the object/data between two contexts. I'm using simply an register to store the pointer (must also work on C too - if boost/config.hpp stuff becomes C conform) - because of performance reasons.
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!)
typo - was only an example for the posting
I'm not sure who you mean by the "user".
user of boost.context
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.
there are no special alignment requirements - the lib does align the address returned by the stack allocator by itself (done inside boost_fcontext_make()).
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.
default_stacksize() was only introduced for more comfort for the lib users. I could also remove this function - but some user might ask what stacksize they should use.
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?).
I looked at the X11 server running on my system - and its stack size was between 200-400kb.
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 :/
'ss' == stack-size Oliver -- NEU: FreePhone - 0ct/min Handyspartarif mit Geld-zurück-Garantie! Jetzt informieren: http://www.gmx.net/de/go/freephone
On Fri, Jan 13, 2012 at 12:54 AM, Oliver Kowalke <oliver.kowalke@gmx.de>wrote: [...]
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
the pattern (using pimpl; type erasure of fn() and args) is also used by boost::thread and boost.threads docu does not document it because it is a implementation detail.
Hmmm...I would consider that an oversight. Any operation (like calling operator new) which changes global state should be documented as such. With operator new, you're (likely) changing the amount of free store space, there's possibly the acquisition of locks (depending on implementation), and, I don't know, maybe some other effects observable outside of the local context. If you put the pimpl in the stack space via the stack allocator, then that changes whatever state the stack allocator has outside of the constructor. Example: If I write a merge sort function that allocates extra temporary scratch space, I would consider the documentation incomplete if it didn't mention where that scratch space is retrieved from. [...]
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.
there are no special alignment requirements - the lib does align the address returned by the stack allocator by itself (done inside boost_fcontext_make()).
Okay. I only suggest to clarify what properties the result of allocate() must satisfy in the concept documentation.
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.
default_stacksize() was only introduced for more comfort for the lib users. I could also remove this function - but some user might ask what stacksize they should use.
I'm simply pointing out that the documented semantics of default_stacksize() strike me as over-specified ("256 kB", regardless of platform), and I was (implicitly) asking, e.g., if you had considered this. Based on your above comment, it appears I didn't make my point clear enough...
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?).
I looked at the X11 server running on my system - and its stack size was between 200-400kb.
Hmmm...suggesting this is a reasonable size on, well, the X11 server running on your system. Do you know, by chance, how to determine the (default?) stack size on Windows (7, specifically)? - Jeff
Hi Jeff, Am 13.01.2012 23:22, schrieb Jeffrey Lee Hellrung, Jr.:
Hmmm...I would consider that an oversight. Any operation (like calling operator new) which changes global state should be documented as such. With operator new, you're (likely) changing the amount of free store space, there's possibly the acquisition of locks (depending on implementation), and, I don't know, maybe some other effects observable outside of the local context. If you put the pimpl in the stack space via the stack allocator, then that changes whatever state the stack allocator has outside of the constructor. Example: If I write a merge sort function that allocates extra temporary scratch space, I would consider the documentation incomplete if it didn't mention where that scratch space is retrieved from. [...]
Hmmm - I've some doubt to buy this. AFAIK the STL documentation does not define how vector, list etc. should be implemented - it is up to the implementor and how it is implemented may change between platforms/vendors.
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.
default_stacksize() was only introduced for more comfort for the lib users. I could also remove this function - but some user might ask what stacksize they should use.
I'm simply pointing out that the documented semantics of default_stacksize() strike me as over-specified ("256 kB", regardless of platform), and I was (implicitly) asking, e.g., if you had considered this. Based on your above comment, it appears I didn't make my point clear enough... so you suggest to remove it? sorry - I don't get it.
Do you know, by chance, how to determine the (default?) stack size on Windows (7, specifically)?
I'm sorry I don't know. It should be the stack size the Windows applications take by average? Maybe 256kB might be OK. regards, Oliver
On Sat, Jan 14, 2012 at 12:38 AM, Oliver Kowalke <oliver.kowalke@gmx.de>wrote:
Hi Jeff,
Am 13.01.2012 23:22, schrieb Jeffrey Lee Hellrung, Jr.:
Hmmm...I would consider that an oversight. Any operation (like calling operator new) which changes global state should be documented as such. With operator new, you're (likely) changing the amount of free store space, there's possibly the acquisition of locks (depending on implementation), and, I don't know, maybe some other effects observable outside of the local context. If you put the pimpl in the stack space via the stack allocator, then that changes whatever state the stack allocator has outside of the constructor. Example: If I write a merge sort function that allocates extra temporary scratch space, I would consider the documentation incomplete if it didn't mention where that scratch space is retrieved from. [...]
Hmmm - I've some doubt to buy this. AFAIK the STL documentation does not define how vector, list etc. should be implemented - it is up to the implementor and how it is implemented may change between platforms/vendors.
std::vector, std::list, and all other STL containers take an Allocator template parameter which dictates how memory should be allocated for the container. The default value for this Allocator parameter is std::allocator, and std::allocator uses operator new.
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.
default_stacksize() was only introduced for more comfort for the lib users. I could also remove this function - but some user might ask what stacksize they should use.
I'm simply pointing out that the documented semantics of default_stacksize() strike me as over-specified ("256 kB", regardless of platform), and I was (implicitly) asking, e.g., if you had considered this. Based on your above comment, it appears I didn't make my point clear enough...
so you suggest to remove it? sorry - I don't get it.
I'm suggesting that you change the documentation to something like "Returns a default stack size, in bytes, which may be platform specific. [Note: The present implementation returns a value of 256 * 1024.]" which leaves the door open to adjust the value up or down for specific platforms in the future. Do you know, by chance, how to determine the
(default?) stack size on Windows (7, specifically)?
I'm sorry I don't know. It should be the stack size the Windows applications take by average? Maybe 256kB might be OK.
I'll look into it a little bit and see if I can figure anything out. - Jeff
on Sat Jan 14 2012, "Jeffrey Lee Hellrung, Jr." <jeffrey.hellrung-AT-gmail.com> wrote:
On Sat, Jan 14, 2012 at 12:38 AM, Oliver Kowalke <oliver.kowalke@gmx.de>wrote:
Hi Jeff,
Am 13.01.2012 23:22, schrieb Jeffrey Lee Hellrung, Jr.:
Hmmm...I would consider that an oversight. Any operation (like calling operator new) which changes global state should be documented as such. With operator new, you're (likely) changing the amount of free store space, there's possibly the acquisition of locks (depending on implementation), and, I don't know, maybe some other effects observable outside of the local context. If you put the pimpl in the stack space via the stack allocator, then that changes whatever state the stack allocator has outside of the constructor. Example: If I write a merge sort function that allocates extra temporary scratch space, I would consider the documentation incomplete if it didn't mention where that scratch space is retrieved from. [...]
Hmmm - I've some doubt to buy this. AFAIK the STL documentation does not define how vector, list etc. should be implemented - it is up to the implementor and how it is implemented may change between platforms/vendors.
std::vector, std::list, and all other STL containers take an Allocator template parameter which dictates how memory should be allocated for the container. The default value for this Allocator parameter is std::allocator, and std::allocator uses operator new.
OK, then, consider all the adaptive algorithms: http://www.sgi.com/tech/stl/stable_sort.html IMO it should be possible to make memory allocation, the use of a second thread (with blocking), temporary file creation, etc. implementation details. That said, it is also polite if a specific implementation gives some details about those details ;-).
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.
default_stacksize() was only introduced for more comfort for the lib users. I could also remove this function - but some user might ask what stacksize they should use.
I'm simply pointing out that the documented semantics of default_stacksize() strike me as over-specified ("256 kB", regardless of platform), and I was (implicitly) asking, e.g., if you had considered this. Based on your above comment, it appears I didn't make my point clear enough...
so you suggest to remove it? sorry - I don't get it.
I'm suggesting that you change the documentation to something like
"Returns a default stack size, in bytes, which may be platform specific. [Note: The present implementation returns a value of 256 * 1024.]"
+1 -- Dave Abrahams BoostPro Computing http://www.boostpro.com
participants (3)
-
Dave Abrahams -
Jeffrey Lee Hellrung, Jr. -
Oliver Kowalke