On Sun, Mar 20, 2011 at 5:18 PM, Vicente BOTET <vicente.botet@wanadoo.fr>wrote:

Hi all,

The review of Oliver Kowalke Context library starts today Mars 21st 2011, and will end on Mars 30th.

Boost.Context is a basic building block library that is used for more higher abstractions. It is used by Boost.Fiber, Boost.Tasklet and Boost.Task from the same author.

I really hope to see your vote and your participation in the discussions on the Boost DEVELOPMENT mailing lists! WARNING !!!!!!!!!!!!! Only on the Boost DEVELOPMENT mailing lists

Please use [context review] as prefix of your post to make easier the review follow up.

You can download a frozen version for the review from the Vault, TinyURL: http://tinyurl.com/5r77rel or

The documentation is available directly here: http://ok73.ok.funpic.de/boost/libs/context/doc/html/

[...]

Please always state in your review, whether you think the library should be accepted as a Boost library!

Yes, provided a number of issues are addressed. Overall, I can imagine there's quite a bit of work behind the scenes necessary to get a context switching abstraction working, and I'm pretty new to context switching and it seems pretty cool, so I think it's awesome that Oliver has put in the time for this library, along with Boost.Fiber and Boost.Coroutine. I'll be looking forward to reviewing these latter two libraries as well! I apologize in advance for not following in detail all the email exchanges regarding Boost.Context, so, Oliver, I would not be surprised if you've already addressed the issues I bring up below. First, I'll explicitly answer the "basic" questions. Additionally please consider giving feedback on the following general

topics:

- What is your evaluation of the design?

Good for a first iteration, but it sounds like there's going to be some major interface changes. My personal take is elaborated on below.

- What is your evaluation of the implementation?

Did not look, as I'm afraid I wouldn't be knowing what I'd be looking at :/ I have never used and am not familiar with ucontext or Windows Fibers, for example. So I'm putting a large amount of faith in Oliver that he's done his due diligence in testing the correctness and performance.

- What is your evaluation of the documentation?

There are several issues, which are probably largely a combination of a non-native English speaker (I'm guessing here!) writing English documentation and me not being familiar with user-level contexts in general. I will give a page-by-page assessment below.

- What is your evaluation of the potential usefulness of the library?

Definitely very useful, at least as a lower level construct on top of which cross-platform fibers and coroutines can be built. I'd expect most Boost users would probably tend to use these higher level abstractions, but it's nice that Oliver factored out the context switching framework.

- Did you try to use the library? With what compiler? Did you have any problems?

No, did not try :( I hope it works!

- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

It didn't take me long to make a full trip through the documentation, so at this point, just a quick glance.

- Are you knowledgeable about the problem domain?

No more knowledgeable than a read through the documentation affords me, so my suggestions should be taken within that context (pun intended). ---------------- My review will parallel the pages of the documentation. I'll try to give grammar/spelling corrections and rewording suggestions as well as more high level evaluations. *** Overview *** (old) *Boost.Context* provides a possibility to switch between different *user-level context* and is intented to be the basis for a higher abstraction like * coroutine* and *fiber*. (new) *Boost.Context* provides [the] possibility to switch between different *user-level context[s]* and is inten[d]ed to be the basis for [] higher abstraction[s] like *coroutine* and *fiber*. (old) A *user-level context* represents the current execution state, including all registers and CPU flags, the instruction pointer, the stack pointer. *boost::context*<http://ok73.ok.funpic.de/boost/libs/context/doc/html/context/context_management/context.html>encapsulates such a *user-level context* and is able to store/restore its associated *user-level context*. This allows multiple execution paths running on a single *thread*using a sort of cooperative scheduling (in contrast a *thread* is preemptively scheduled) - the running *boost::context*<http://ok73.ok.funpic.de/boost/libs/context/doc/html/context/context_management/context.html>decides explicitly when its yields to allow another *boost::context*<http://ok73.ok.funpic.de/boost/libs/context/doc/html/context/context_management/context.html>to run ( *user-level context* switching). A context can only run on a single *thread*at any point in time but may be migrated between *thread*. (new) A *user-level context* represents the current execution state, including all registers and CPU flags, the instruction pointer, [and] the stack pointer. *boost::context*<http://ok73.ok.funpic.de/boost/libs/context/doc/html/context/context_management/context.html>encapsulates such a *user-level context* and is able to store/restore its associated *user-level context*. This allows multiple execution paths running on a single *thread*[to use] [] cooperative scheduling ([by] contrast[,] a *thread* is preemptively scheduled) - the running *boost::context*<http://ok73.ok.funpic.de/boost/libs/context/doc/html/context/context_management/context.html>decides explicitly when it[] yields to [] another *boost::context*<http://ok73.ok.funpic.de/boost/libs/context/doc/html/context/context_management/context.html>[] ( *user-level context* switching). A context can only run on a single *thread*at any point in time but may be migrated between *thread[s]*. [I would change "store/restore" to something like "store/load" or "save/restore". "active boost::context" might be better than "running boost::context".] At this point, given that I'm not at all familiar with context technology and terminology, the "user-level" in "user-level context" seems superfluous. I can only guess that there are "kernel-level" contexts... (old) A context switch between threads costs usally thousends of CPU cycles on x86 compared to a *user-level context* switch with few hundreds of cycles. (new) A context switch between threads [generally] costs [] thous[a]nds of CPU cycles on x86[; in contrast,] a *user-level context* switch [costs only a] few hundred[] [] cycles. (old) *Boost.Context* differes to *setjmp()*/*longjmp()* in the fact that the C99 standard does not require that *longjmp()* preserves the current stack frame, e. g. jumping into a function which was exited via a call to * longjmp()* is undefined. (new) *Boost.Context* differ[]s [from] *setjmp()*/*longjmp()* in [] that the C99 standard does not require [] *longjmp()* [to] preserve[] the current stack frame[;] e.[]g.[,] jumping into a function which was exited via a call to *longjmp()* is undefined. I presume setjmp and longjmp are C99 functions? If so, perhaps prefix their introduction as such, e.g., "...differs from the C99 functions setjump()/longjump()...". (old) *Boost.Context* allows to select the mechanism provided by the operating system (ucontext on UNIX, Windows Fibers on Windows) or a faster implementation provided by this library (fcontext using assembler). (new) *Boost.Context* allows [one] to select [either] the mechanism provided by the operating system (ucontext on UNIX[;] Windows Fibers on Windows) or a faster implementation provided by this library (fcontext[,] using assembler). This sentence doesn't immediately make it clear what these selections are *for*. Perhaps better phrased as "Boost.Context allows one to choose either of two underlying context implementations: that provided by the operating system (ucontext on UNIX; Windows Fibers on Windows); or a faster assembler [or assembly?] implementation provided by this library (fcontext)." Would it be better (e.g., more accurate) to say that "ucontext is deprecated and may not be available", rather than "functions of ucontext are deprecated and may not be available"? (old) *Boost.Context* depends uppon *Boost.Move*. (new) *Boost.Context* depends [on] *Boost.Move*. Overall, this page does indeed give a good overview of the library. *** How to build and install *** So, first you claim "Boost.Context provides an abstraction (boost::contexts::native_impl)...", then go on to say that "...boost::contexts::native_impl might not be available". I know what you mean, but these still seem to be contradictory statements. (old) The fast context swapping implementation (boost::contexts::asm_impl) using assembler is only available on certain platforms. Currently only gcc, msvc and icc are supported for boost::contexts::asm_impl as well gas (GNU assembler) on UNIX and masm (Microsoft macro assembler) on Windows are required. (new) The fast context swapping implementation (boost::contexts::asm_impl) using assembler is only available on certain platforms. Currently only gcc, msvc[,] and icc are supported for boost::contexts::asm_impl[, and] gas (GNU assembler[, on UNIX)] [or] masm (Microsoft macro assembler[, on Windows)] are required. Yikes, I'm scared by all these build parameters. Any way we can make building one implementation or the other easier? Or maybe expand this section on how to build the fcontext implementation? I'm a bjam dummy. I just run bootstrap and bjam to build boost libraries, and use Visual Studio on Windows and scons on Linux. I presume the middle column of the table are bjam invokations...? What if I don't use bjam to build stuff? I'm a little uncomfortable with have two context implementations, where neither is actually available on all platforms. I thought Boost.Context was suppose to provide a cross-platform abstraction of contexts, but by exposing the implementation choice to the user, it seems we're losing some "cross-platform'ability". Jumping ahead, it doesn't help that the documented default implementation is the asm_impl, which suggests that boost::context won't work with the default template parameters on platforms where only native_impl is an option. I suppose this can be mitigated somewhat by providing preprocessor definitions or typedefs indicating which context implementations are available, but I think it would be preferable for Boost.Context to just choose the best implementation for each platform. It seems the only (documented) drawback with using asm_impl is failure to preserve signal masks and inability to be called by asynchronous signal handlers. Are these sufficiently useful to preclude hiding which context implementation is used? Any code that requires signal mask preservation and/or asynchronous signal handlers would have to conditionally use those anyway to remain cross-platform, right? Is this what you meant "modifications" email by "context simple class/not a template"? *** Context Management *** I presume a good deal of the interface of boost::context will change, as, per your "modifications" email: - context will become moveable. - context will become a non-template class (meaning no dependence on the stack type or the implementation choice, I suppose?) - context will no longer manage the stack. ...all of which I think are positive changes. As such, it seems a little silly to go over this in too much detail. However, I still do have a few comments. The parameters in the context::context( void (*)(void*), context const&, void*, stack_type&& ) seem out-of-order to me...especially separating the void* parameter from the function pointer parameter. I suggest context::context( void (*)(void*), void*, context const& nxt, stack_type&& ) or (probably better) context::context( void (*)(void*), void*, stack_type&&, context const& ). And, minor quibble, I don't like using "nxt" as an abbreviation for "next", as it only saves one letter :) One would infer from the example (and this is confirmed by the documentation) that the default constructor sets the boost::context object to refer to the current context the object resides in, so, in fact, boost::context::current() does *not* have to be called to get the current user-level context. As such, I would rephrase "To get the current user-level context, boost::context::current() has to be called" to "One can get the current user-level context via boost::context::current()" (I'm also partial to active predicates rather than passive ones). Also, should this be boost::context<>::current() (at least as long as boost::context has template parameters)? And why is boost::context::current() not listed among the other member functions of boost::context? I would like to see a more elaborate example demonstrating context switching. Maybe some with print statements and some expected output. The single example in the documentation is weak at best. I would guess, but I'd have to think hard about it, that any program which uses boost::context must create at least two boost::context objects, and at least one of these objects must reference the current context. Is this the case? To switch back to the stalled context before the switched-to context's function returns, the stalled context must be passed (through the void* argument) to the switched-to context's function, right? Such observations might be useful to include in the documentation. Maybe even a bare-bones, super simple coroutine example might be worthwhile. I would prefer "switch" (well, actually it would have to be "switch_") or "switch_to" over "jump_to", since this executes a "context switch" (that's the correct use of terminology, right?). *** Class context *** I would prefer (in this case) to have the entire class reference on a single page, rather than a separate page devoted to each constructor and member function. Same comment applies to the protected_stack reference. *** Context Stack *** I don't know how much of this stack documentation will be preserved, but I'll give my comments on the stack documentation anyway. The stack concept requirements should be documented separately from the protected_stack reference. Also, please include semantic requirements in addition to syntactic requirements. I had a number of issues with the interface of protected_stack: - No allocator abstraction. I'm guessing that the std allocator concept is insufficient to provide the allocator interface you'd need, but that shouldn't stop you from providing an allocator abstraction that meets your needs. Indeed, it's not clear *where* a protected_stack object allocates its memory from; at the very least *this* should be documented. - I don't know that I agree that a stack must protect against exceeding the stacksize (although protected_stack certainly must!)...surely there are situations where you *know* you have enough stack space for your particular application, and hence guard pages would be unnecessary. It doesn't look like there's a way to determine the amount of free space / used space in a stack...is such a thing possible and/or useful? - protected_stack::protected_stack(std::size_t): Is there any way I can get the range of stack sizes supported by the operating system? This constructor seems pretty unusable without such information... - What are the advantages of making protected_stack default constructible? - operator [unspecified-bool-type() and operator!() were confusing to me at first, as it left me wondering how a protected_stack object could be anything other than a stack! I think, if default-constructible stacks (i.e., not-a-stack stacks) are deemed useful or necessary, they should be called something other than "not-a-stack", and more descriptively named member functions should be used to query the non-a-stack state. *** Performance *** For the assembler implementation of context switching, is it possible to explicitly calculate the number of cycles, via a summing of the cycle counts of the instructions? I don't know x86 assembly at all; my only experience is with Z80 assembly, and such summing of instruction cycles is possible there. Also, is the performance testing code available? I think it should be, and it should either be included in the documentation or referenced. ---------------- Okay, that's my review. I hope the comments help, and thanks again, Oliver, for contributing Boost.Context (and, soon, Boost.Fiber and Boost.Coroutine) to Boost! - Jeff