[boost] [context review] Quick Review

Subject: [boost] [context review] Quick Review
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2012-01-03 14:57:11

Here's my quick review of the proposed Boost.Context library.

I'm not familiar with the kernel-level machinations involved to save and restore the execution state managed by the context class, so I cannot comment on such implementation details and whether they satisfy the acceptance criteria.  Consequently, I didn't bother to look at the implementation.

The examples are good, but the discussion or comments are somewhat lacking.  For example, the Enumerator example is based upon an enumerator class type which is not commented and has very little introductory text.  There's a link to C# documentation on the yield keyword, but no association of enumerator's yield_return() to C#'s "yield return" statement or yield_break() to "yield break".  Those associations can be inferred easily enough, but are better stated explicitly.

The enumerator example is further confused by indicating that a call to yield_return() in the power derivate returns a value to the while loop in main().  What's missing is that yield_return() calls context::suspend() on enumerator's context data member which returns control to enumerator<T>::get(), which is called by main()'s while loop.

The entire enumerator example should be introduced by discussing the power class first, without implementation details:

   class power
   {
   public:
      power(int _base, int _exponent);

      bool
      get(int & _value);
   };

Then, the example can show main(), since the above interface is sufficient.  That done, the example can turn the reader's attention to how to implement power by referring to the C# yield documentation example and then relating it to Boost.Context.  That will lead to the enumerator class.  (Making enumerator a class template complicates matters more than necessary, BTW.)  Finally, you should show expected output.

I'd like to see a performance comparison of using Boost.Context versus Chris Kohlhoff's coroutines.  Can his coroutines be replaced with yours without loss of efficiency?  Would it require a different stack allocator to match the performance of his coroutines?  (Developing the example in that direction would be useful to illustrate writing a custom stack allocator.)

I don't find anything related to the Movable Stack concept suggested in the acceptance criteria.  The documentation of the StackAllocator is lacking, but I've suggested a rewrite below.  With that rewrite, I think it satisfies the Stack concept required for acceptance.

There are no performance figures for Windows.  Shouldn't there be?

I think I recall that the build system doesn't support the necessary information, by default, to permit building an ABI compatible version of the library.  If that's not the case, then the "How to build and install" section needs more work.  Either way, the Build acceptance criteria has not quite been met yet.

The _suggested_ callback-based linking was not adopted in lieu of fc_link.  There should be some rationale or a FAQ for that.

I've called for still more work on the documentation, but as it stands, with the concept changes I've suggested, I'm satisfied that Oliver has met the documentation acceptance criteria.

_____________________________________________
Documentation comments follow.

_______________
Overview

This section is useful and clear, except for the second sentence of the third paragraph.  It doesn't connect to the library in question as it ought and is worded a little unclearly.  I'd suggest the following instead:

"By contrast, transferring control within a single thread, as _context_ does, requires only a few hundred CPU cycles since it involves no system calls."

You might also add something like, "which makes them useful in more contexts, particularly when implementing higher level execution control systems", to that sentence.

What's missing is a Motivation section or more information in the Overview to better explain why someone would want to use Boost.Context versus other options (whatever they may be).

_______________
How to build and install

There should be an introductory paragraph, something like the following:

"Boost.Context must be built for the particular compiler(s) and CPU architecture(s) being targeted.  Each combination requires specific build command arguments as documented below."

The bullets at the top of the page are only needed until the library is accepted, so be sure to remove them upon acceptance.

"On POSIX and Windows systems, GNU AS and MASM are respectively required for the assembler implementation of Boost.Context" is awkward.  I suggest the following instead:

"Boost.Context includes assembly code and, therefore, requires GNU AS for supported POSIX systems, and MASM for Windows systems."

The Note is a little awkward and includes a spelling error.  I suggest the following instead:

"The architecture, instruction set, and address model are optional Boost.Build properties that must be given on the bjam command line, as shown in the table below."

_______________
Executing a context

3rd para: s/a function or callable object/a _context-function_/

5th para: this should be merged with para 4 and "and might be changed" should be inside the parentheses or I don't understand how it "might be changed".

6th para, 4th sent:  Add the following to the end: ", after saving the new state of the original context"

6th para, last sent: Is there anything that ensures start() is called first and only once?

Example:

  - The control transfer comments in fn() could be a little clearer:

      // transfer execution control back to main()
      ...
      // execution control transferred back from main()

  - The first "main() calls context ctx" would be better as "main() starts context ctx"

  - The comment, "// ctx.suspend() was called", in the while loop is only accurate until the for loop, in fn(), terminates, right?

  - Expected output is not shown

_______________
Transfer of data

Example:

  - Missing comments discussing the pointers on the receiving end.  For example, in fn(), I'd expect the following:

    void * vp = ctx.suspend(&i);
    // vp is &x from main()'s call to ctx.resume(&x)
    int j = *static_cast<int *>(vp);
    // j == 10 because x == 10 in main()

  - Expected output is not shown

_______________
Exceptions in context-function

s/emits/emits/

Is forced_unwind a documented exception type?  That is, can it be caught explicitly?  If so, the reference to "forced_unwind" should be a link to its documentation and the following might be documented as legitimate:

   try
   {
      // code that might throw
   }
   catch (forced_unwind)
   {
      throw;
   }
   catch (...)
   {
      // possibly not rethrow pending exception
   }

_______________
Terminating context

Shouldn't this section name be something more like, "Handling execution context termination"?

2nd example: remove the "Done" line.  The "never reached" comment can then be applied to "return EXIT_SUCCESS".

_______________
Chaining contexts

The first two sentences should be merged something like the following:

"Boost.Context provides the ability to chain context instances by passing a reference to another context, other than _not-a-context,_ as the last constructor argument."

You note that the application terminates or control is transferred back to the last invocation of context::resume().  Might not the transfer also be to context::start() (if suspend()/resume() are never called)?

Important: I think you mean, "context::start() must not be called on a context that is a link in the chain begun by another context.  That is, call context::start() only on the first context in a chain."

_______________
Unwinding stack

Name this section "Stack unwinding"

2nd para, 1st bullet: Should "not-a-context" be italicized?

Speaking of italics, "context" is sometimes italicized and blue and other times plain.

Example: Expected output is not shown

Important: s/exception!/exceptions!/

_______________
context.html

Variously through this page: s/an context/a context/ or s/an context/an instance/

s/unwinds the stack before destructing it/unwinds the stack first/

s/if precondition is not fulfilled/if a precondition is not satisfied/

s/nxt is linked/nxt becomes a subsequent link in a chain/

s/the execution control/execution control/

s/The execution control/Execution control/

s/Destroyes/Destroys/

Is the move assignment operator safe against self-move assignment?

start():

   s/with wich/with which/
   s/When the function returns/Upon return/ (to avoid confusion about whether start() or fn() has returned)

resume():

   s/back resuming/back, resuming/
   s/ (means suspend()/, which means that suspend() returns/ (note the formatting of "returns" should be normal, not code)
   "[[Returns:]" is malformed
   Effects could be rewritten, something like the following, to be clearer:

"None, if *this was not suspended by a previous call to suspend().  Otherwise, execution control is transferred back to the caller of suspend(), thereby resuming *this.  The argument, vp, will be returned by the previous call to suspend()."

suspend():

   I think Effects is incomplete.  There are spelling errors.  How about the following?

"Transfers execution control to the caller of the most recent call to start() or resume().  The argument, vp, will be returned by start() or resume()."

unwind_stack():

   Effects: suggestion:

"Destroys all objects allocated on the stack, owned by *this, in the reverse order of their allocation."

Non-member function swap():

   Effects: suggestion:  "As if l.swap(r)."

_______________
Stack allocation

1st para:

  - "stack allocator" should probably be italicized rather than rendered as code.

  - Concept names are usually rendered with camelcase.  Thus, s/stack-allocator/StackAllocator/.

2nd para: This should be the start of a "StackAllocator Concept" section:

"StackAllocator Concept

"A stack allocator must satisfy the StackAllocator concept requirements shown in the following table, in which a is an object of a StackAllocator type, p is a void *, and s is a std::size_t:
"
"   Expression         | Return type | Notes
"   -----------------------------------------------
"   a.allocate(s)      | void *      | Returns a pointer to s bytes allocated from the stack
"   -----------------------------------------------
"   a.deallocate(p, s) | void        | Deallocates s bytes of memory beginning at p, a pointer previously returned by alloc.allocate()"

The current documentation of allocate() and deallocate() are no longer needed.

Important:  This should be rephrased in terms of the concept to something like the following:

"The implementation of allocate() might include logic to protect against exceeding the context's available stack size rather than leaving it as undefined behavior."

You might add a note that the result of calling deallocate() with a pointer not returned by allocate() results in undefined behavior.

_______________
Helper functions

1st para: s/an easy/easy/

_______________
Low level API (boost_fcontext_t)

You refer to "ucontext_t" with no link or description.

The example code is out of place without first seeing the documentation of boost_fcontext_t and related functions.  Following the -> navigation order, the example comes first.  Commenting the example and providing an introduction explaining what it illustrates would work.

There is no expected output shown for the example.

Presumably, the low level API is the part to port to new platforms, yet you don't state that.

_______________
Struct boost_fcontext_t and related functions

There is no documentation for boost_fcontext_t, particularly as it appears to be just an alias for boost_fcontext.  I thought for a moment that you intend to permit different platforms to redefine the typedef to refer to different structures, as needed, but you've documented boost_fcontext_stack's ss_base and ss_limit fields, which suggests that you intend those to be part of the low level, yet documented, API.  The result is that I can't figure out the purpose of the typedef.

The "fc_" prefix makes sense, since it is used in boost_fcontext.  By contrast, the "ss_" prefix does not make sense.  I could see "fcs_", for example.

ss_limit: Is the stack defined by [ss_base, ss_limit) or [ss_base, ss_limit]?  "Pointer to the bottom of the stack" is ambiguous.

fc_stack: The description is a little off.  How about, "Tracks the memory for the context's stack"?

fc_link: The description could be improved: "The address of the next context link in a chain, if any."

boost_fcontext_start():  Effects are a little confusing.  My suggestion:

"Stores the current context data (stack pointer, instruction pointer, and CPU registers) to *ofc and restores the context data from *nfc, which implies jumping to *nfc's execution context.  This function must be called when first entering *nfc's execution context."

boost_fcontext_start(): Need a "Returns:"

"The result of calling boost_fcontext_jump()."

boost_fcontext_jump(): This likewise needs some work:

Effects: "Stores the current context data (stack pointer, instruction pointer, and CPU registers) to *ofc and restores the context data from *nfc, which implies jumping to *nfc's execution context.  The pointer argument, vp, is passed to the current context to be returned by the most recent call to boost_fcontext_start() or boost_fcontext_jump() in the same thread."

Returns: "The third pointer argument passed to the most recent call to boost_fcontext_jump(), if any."

boost_fcontext_make(): Precondition: s/finished /finishes, / and s/inside/in/

boost_fcontext_make(): Effects: s/if the context is activated next time/when the context is activated next/

_______________
Continuation

1st para: s/from Java/from the Java/

2nd para: s/for functors objects/for a functor/

3rd para: You say the co-routines run "in parallel", but there is no parallelism, so you should be clear about what does occur.

_______________
ASIO

2nd para: s/a echo/an echo/

3rd para: I quickly got lost in this description because it assumes familiarity with code presented later.  It also assumes familiarity with control flow in Asio which can be described rather than assumed.

3rd para:
   s/reentered a/reentered, a/
   s/a asynch./an async/

4th para:
   s/adavantage/advantage/
   s/bind()-version/bind() version/
   s/easier implemented/more easily implemented/
   s/For instance if/For instance, if/
   s/a message is partialy received/only part of a message is received/
   s/parser or serviced)/parsed or serviced),/

_______________
Performance

The bullets are too terse.  Perhaps the following captures your intent:

"Performance of Boost.Context was measured on the platforms shown in the following table.  Performance measurements were taken using rdtsc, with overhead corrections, on x86 platforms.  In each case, stack protection was active, cache warm-up was accounted for, and the one running thread was pinned to a single CPU.  The code was compiled using the build options, 'variant=release cxxflags=-DBOOST_DISABLE_ASSERTS'.

"The numbers in the table are the number of cycles per iteration, based upon an average computed over 1000 iterations."

_______________
Rationale

NoThrow guarantee: I think you meant the following:

"Member functions of context do not throw exceptions but do require that certain preconditions are satisfied: context jumps can only be made by instances which are not a _not-a-context_ and which are not complete."

Pimpl idiom, 1st para: s/ it was required to separate both features in separate classes via pimpl idiom/, the use of the Pimpl Idiom was necessary/

Pimpl idiom, 3rd para: I suggest the following:

"The Pimpl Idiom allows type-erasing the _stack-allocator_ type."

Unwinding: s/resource/resources/ and s/a exception/an exception/

Protecting the stack:  There are many changes to make here, so I'll suggest this rewrite:

"Because the stack's size is fixed -- there is no support for split stacks -- it is important to protect against exceeding the stack's bounds.  Otherwise, in the best case, overrunning the stack's memory will result in a segmentation fault or access violation and, in the worst case, the application's memory will be overwritten.  stack_allocator appends a guard page to the stack to help detect overruns.  The guard page consumes no physical memory, but generates a segmentation fault or access violation on access to the virtual memory addresses within it."

setjmp()/longjmp():
   s/The C99/C99/
   s/frame, e.g. jumping/frame.  Therefore, jumping/

ucontext_t:

I don't know why anything in this section is included after the first sentence.

ucontext_t, para 1: s/makecontext():/makecontext() is:/

ucontext_t, para 2: s/alter/later/

I would omit the "The signature of func is" part.  That's clear from the makecontext() function pointer argument.

Windows fibers: s/befor/before/

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer                     using std::disclaimer;
Dev Tools & Components
Susquehanna International Group, LLP  http://www.sig.com

________________________________

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.