Re: [boost] Boost.Fiber review January 6-15

Subject: Re: [boost] Boost.Fiber review January 6-15
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2014-01-09 13:54:37

Le 09/01/14 08:42, Oliver Kowalke a écrit :
> 2014/1/8 Vicente J. Botet Escriba <vicente.botet_at_[hidden]>
>
>> Hi Oliver,
>>
> hello Vicente
>
>
>> The interface must at least follows the interface of the standard thread
>> library and if there are some limitations, they must be explicitly
>> documeted.
>> Any difference respect Boost.Thread must also documented and the rational
>> explained.
>>
> OK - section rational
>
>
>> std::thread is not copyable by design, that is only one owner. WHy
>> boost::fibers::fiber is copyable?
>>
> boost::fibers::fiber should be movable only - it is derived from
> boost::noncopyable and uses BOOST_MOVABLE_BUT_NOT_COPYABLE
Sorry, I don't find now from where I read that fiber is copyable. Maybe 
I was tired :(
>
>
>> Why the exceptions throw by the function given to a fiber is consumed by
>> the framework instead of terminate the program as std::thread?
>>
> the trampoline-function used for the context does following:
> - in a try-cactch-block execute the fiber-code (fiber-function given by the
> user)
> - catch exception forced_unwind from boost.coroutine -> release the fiber
> and continue unwinding the stack
> - catch fiber_interrupted -> stored inside boost::exception_ptr (might be
> re-thrown in fiber::join() )
> - catch all other exceptions and call std::terminate()
>
> I though this would let to an equivalent behaviour as std::thread
>
Then you should be more explicit in this paragraph
"Exceptions thrown by the function or callable object passed to the 
|fiber| 
<http://olk.github.io/libs/fiber/doc/html/fiber/fiber_mgmt/fiber.html#class_fiber> 
constructor are consumed by the framework. If you need to know which 
exception was thrown, use |future<>| 
<http://olk.github.io/libs/fiber/doc/html/fiber/synchronization/futures/future.html#class_future> 
and |packaged_task<>| 
<http://olk.github.io/libs/fiber/doc/html/fiber/synchronization/futures/packaged_task.html#class_packaged_task>. 
"
>> Which exception is thrown when the Error Conditions:resource_deadlock_would_occurand
>> invalid_argument are signaled?
>>
> I use BOOST_ASSERT instead of exception
Where is this documented?
>
>> Why priority and thread_affinity are not part of the fiber attributes?
>>
> you referre to class attributes passed to fiber's ctor? this class is a
> vehicle for passing special parameter (for instance stack-size) to
> boost::coroutine - if you use
> the segmented-stack feature you usually don't need it.
> priority() and thread_affinity() are member-functions of
> boost::fibers::fiber to make the modifications those parameters for an
> instance more explicit
What do you think about adding them to the class atributes also?
>
>> The interface let me think that the affinity can be changed by the owned
>> of the thread and the thred itself. Is this by design?
>>
> thread_affinity() expresses if the fiber is bound to the thread - this is
> required for fiber-stealing, e.g. a fiber which is bound to its running
> thread
> will not be selected as candidate for fiber-stealing.
This doesn't respond to my question. Why the change of thread_affinity 
need to be changed by the thread itself and by the fiber owner?
>
>
>> Please don't document get/set functions as thread_affinity altogether.
>>
> OK
>
>
>> The safe_bool idiom should be replaced by a explict operator bool.
>>
> hmm - I thought using 'operator bool' is dangerous (I remind on some
> discussion of this issue by Scott Meyers).
Could you explain why it is dangerous and how it is more dangerous than 
using the safe bool idiom?
> do you have other infos?
>
>
>> Why is the scheduling algorithm global? Could it be threads specific?
>
> It is thread specific (using boost::thread_specific_ptr)
How the user selects the thread to which the scheduling algorithm is 
applied? the current thread?
If yes, what about adding the function on a this_thread namespace?

     boost::fibers::this_thread::set_scheduling_algorithm(  &  mfs);

>
>
>> BTW i sthere an exmple showing  the |thread_specific_ptr trick mentioned
>> on the documentation. |
>>
> which trick? maybe you referring to
"Note:

    |set_scheduling_algorithm()| does /not/ take ownership of the passed
    |algorithm*|: *Boost.Fiber* does not claim responsibility for the
    lifespan of the referenced |scheduler| object. The caller must
    eventually destroy the passed |scheduler|, just as it must allocate
    it in the first place. (Storing the pointer in a
    |boost::thread_specific_ptr| is one way to ensure that the instance
    is destroyed on thread termination.)"

> algorithm *
> scheduler::instance()
> {
>      if ( ! instance_.get() )
>      {
>          default_algo_.reset( new round_robin() );
>          instance_.reset( default_algo_.get() );
>      }
>      return instance_.get();
> }
>
>
>> Why the time related function are limited to a specific clock?
>>
> it is a typedef to steady_clock (if avaliable) or system_clock - one of the
> main reasons is that you would have made the schedulers be templates.

Maybe schedulers need to take care of a single time_point, but the other 
classes should provide an time related interface using any clock.
>
>
>> The interface of fiber_group based on old and deprecated thread_group is
>> not based on move semantics. Have you take a look at the proposal
>> N3711 <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3711.pdf>
>> Task Groups As a Lower Level C++ Library Solution To Fork-Join Parallelism
>>
> no - but I've no problems to remove it form the library

>
>
>> Maybe it is worth adapting it to fibers.
>> **
>> Boost.Thread has deprecated the use of the nested type scoped_lock as it
>> introduce unnecessary dependencies. DO you think it is worth maintaining it?
>>
> oh - I wasn't aware of this issue - I've no preferrence to scoped_lock
> (which is a typedef to unique_lock, AFAIK)
>
>
>> I made some adaptations to boost::barrier that could also have a sens for
>> fibers.
>
> OK - what are those adaptations?
See
http://www.boost.org/doc/libs/1_55_0/doc/html/thread/synchronization.html#thread.synchronization.barriers 
and
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3817.html#barrier_operations
>
>
>> I don't know if a single class could be defined that takes care of both
>> contexts for high level classes as the barrier?
>>
> a problem is raised by the mutex implementations - thread's mutexes are
> blocking the thread while fiber's mutexes do
> only suspend the current fiber while keep the thread running (so other
> fibers are able to run instead)
>
> I was thinking on a combination of sync. primitives for threads and fibers
> too , but it is not that easy to implement (with a clean interface)
Ok. Glad to see that you have tried it.
>
>> Boost.Thread would deliver two synchronized bounded and unbounded queue
>> soon based on
>> N3533 <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3533.html>
>> C++ Concurrent Queues
>>
> OK
>
>
>> Have you tried to follow the same interface?
>>
> I did look at the proposal of C++ Concurrent Queues - but I didn't adapt
> the complete interface.
> for instance Element queue::value_pop(); -> queue_op_status queue::pop(
> Element &);
>
Could you explain the rationale?

   Element queue::value_pop();

can be used with no default constructible types

while

   queue_op_status queue::pop(Element &);

not.

Best,
Vicente