Re: [boost] [context review] Review starts today, Mars 21st 2011

Subject: Re: [boost] [context review] Review starts today, Mars 21st 2011
From: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2011-03-30 02:13:26

On Sun, Mar 20, 2011 at 5:18 PM, Vicente BOTET <vicente.botet_at_[hidden]>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