[boost] [context review] Formal Review

Subject: [boost] [context review] Formal Review
From: Artyom (artyomtnk_at_[hidden])
Date: 2011-03-22 16:50:42

Hello,

This is my formal review.

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

Yes, it should be a part of boost.

(the alternative version with improved f/u-context support)

However following issues should be addressed ASAP:

1. Documentation must be improved with much better description, rationale,
   examples and design notes and description of member functions,
   stack unwinding in case of context destruction and so on.

   See notes below.

2. Current build is too verbose.

   All relevant options should be configured automatically by
   default without user specifying them. I understand that it
   is limitation of BBv2 but it MUST be addressed in the
   official release.

Now Full Review:
----------------

> What is your evaluation of the design?

I had issues with Design of fcontext/ucontext but it was
addressed by Oliver in very fast and I think correct way
so I have no more problems with it.

Also I have important concerns about verbosity of build system use.
I've discussed this before. BBv2 improvements required.

> What is your evaluation of the implementation?

The heaviest part of implementation is in assembly so I can't
say too much.

C++ code looks fine.

Current issues:

Extern "C" required in context_ucontext.hpp

  typedef void ( * stack_fn)();

  should be extern "C" you need to pass pointers
  to makecontext as extern "C" function.

Asm implementations:

Add  "namespace" names like boost_get_context instead of
get_context. It would prevent names collision and in general
allows tools like BCP to rename namespace to use two versions
of boost library.

> What is your evaluation of the documentation?

This is the weakest point of this library.

The documentation is not sufficient. What do I miss:

a) Context destruction and unwinding. When I tried to implement
   yield like iterator I had problem with unwinding the
   stack of auxiliary context when the program does not
   get execution.

   I should add some throwing in yield and some flags
   to make sure that stack is unwind in destruction
   of iterator.

   The behavior should be documented and examples should be provided.

   I explain more:

   Consider implementation like
   ...
   void iterate()
   {
     object foo; // may be not destroyed
 
     for(;;) {
        global_value = local_value++;
        if(local_value > 100000)
            return;
        else
            my_context.jump_to(other_context);
     }
   }
   ...
   int operator++() {
      other_context.jump(my_context);
      return global_value;
   }
   ...
   iterator it;
   while(it++ < 10)
     ;
   ...

   Now what happens when other_context does not return
   execution to my_context when gloabl_value becomes 10.

   Then foo object is not get destroyed. So I need explicity
   switch to my_context and throw something to make stack
   unwind procedure work and foo is not got destroyed.

   Such things should be documented and example how to use
   it should be provided (i.e. what is correct way to unwind
   the stack).

b) Design rationale is missing

c) I'd like to see comparison of features ucontext and fcontext provide.
   A table of limitations/implementations/platforms like

   Limitations\Methond    ucontext    fcontext
   Signals                ... 
   Performance    
   ...

d) Description of member functions of boost::context is too brief.

e) Implementation details description should be given.

f) Better examples should be provided, I've attached few programs
   I used to test Boost.Context that implements yield like iterator
   and using context with Boost.Asio in thread like run and such
   examples should be given.

   If you want to use the code of the exapmples, consider
   them under BSL.

g) The option context-impl is removed in alternative version
   and mistakenly was left in the docs.

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

It is useful. I implemented a simple client server using Boost.Asio
and Boost.Context that switch contexts on async requests
and it was quite easy and straight-forward and brings very
interesting paradigm to C++.

I also had implemented a yielding iterator and it is was very nice, I
like a fact that I have such concepts in C++.

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

I tested in on Linux  x86_64 with gcc-4.4

I had problems that executable crashes using shared object libboost_concept.so
and fcontext (when it is not compiled with -fPIC which is correct!)

The author confirmed the bug and fixed it, no more problems seen.

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

I mostly tried to implement several tools like iterator and use
it with Asio. I run some benchmarks and was quite impressed with fcontext
performance.

I spend about 6 hours on reviewing this library

I implemented a simple client server using Asio and Context where in one case
one side was reading and other writing and in other case it was
echo server and client benchmark results were quite nice:

Reader/Writeer Asio
-------------------

Real-Threads, 2 CPU    0.95s
Real-Threads, 1 CPU    1.53s
ucontext               3.58s
fcontext               2.90s
Asio-Async             2.84s

Echo Asio
----------

Real-Threads, 2 CPU     12.30s
Real-Threads, 1 CPU      6.58s
ucontext                 7.24s
fcontext                 5.23s
Asio-Async               5.24s

So I think it is very good base to start from.

> Are you knowledgeable about the problem domain?

I hadn't used ucontext and fibers before, but I'm
familiar with event driven programming and threading.

I have large experience and asynchronous programming
and implemented once threading support for MS DOS and
for Z80.

Additional Unrelated Note:
--------------------------

Would you consider to create C version of Boost.Context
or create C API.

It is very useful to have fast setcontext/makecontext
operations in C because it may be useful for too many
systems where C API is must easier to integrate then C++.

Best Regards,
  Artyom Beilis