Re: [boost] [chrono] [thread] boost.chrono changes to boost::condition_variables

Subject: Re: [boost] [chrono] [thread] boost.chrono changes to boost::condition_variables
From: Howard Hinnant (howard.hinnant_at_[hidden])
Date: 2010-07-23 12:30:18

I'll only address what the draft standard says, and what are potential changes.  I leave it to others to decide what actually happens to the boost library...

On Jul 23, 2010, at 10:18 AM, Morgan wrote:

> Currently, the condition_variable implementation (existing and proposed) is
> strongly bound to the system clock for waits with time-outs; i.e. the "system
> clock" is used for timed waits irrespective of your desired time base.
> 
> All wait_duration/rel_time overloads always assume CLOCK_REALTIME.
> 
>        template<typename duration_type>
>        bool condition_variable::timed_wait(unique_lock<mutex>& m,
>             duration_type const& wait_duration)
>        {
>            return timed_wait(m,boost::get_system_time()+wait_duration);
>        }
> 
>        template <class Rep, class Period>
>        bool condition_variable::wait_for(unique_lock<mutex>& lock, 
>             const chrono::duration<Rep, Period>& rel_time)
>        {
>            return (timed_wait(lock,
>                    convert_to<posix_time::time_duration>(rel_time))
>        }
> 
> Even if you supply a chono::monotonic_clock::time_point (CLOCK_MONOTONIC) it is
> erroneously converted to a CLOCK_REALTIME.
> 
>        template <class Clock, class Duration>
> 
>        bool wait_until(unique_lock<mutex>& lock, const
>             chrono::time_point<Clock, Duration>& abs_time)
> 	{
>            return timed_wait(lock, convert_to<system_time>(abs_time), pred);
>        }
> 
>        bool condition_variable::timed_wait(unique_lock<mutex>& m,
>             boost::system_time const& wait_until)
> 
> The implementation _always_ assumes the POSIX CLOCK_REALTIME time base before
> forwarding its calculations to pthread_cond_timedwait.  This creates bugs when
> you wish the condition_variable to operate in eg the CLOCK_MONOTONIC time base
> by using chono::monotonic_clock.

[thread.req.timing]/2 says:

> The member functions whose names end in _for take an argument that specifies a relative time. Implementations should use a monotonic clock to measure time for these functions. [ Note: Implementations are not required to use a monotonic clock because such a clock may not be available. — end note ]

In English:  wait_for should always use a monotonic clock under the hood, unless the platform doesn't have one.

More info:  A future draft of the standard may change to say that platforms must have a monotonic clock.  This has been proposed, but not yet debated.  The debate will probably happen two weeks from now.

> In my view, the new boost::condition_variable API is incomplete, and lacks the
> ability to properly bind the condvar to one of the new clock types.  Ideally,
> you would want time calculations to remain in the time base you selected. ie, it
> should use <clock_type>::now() for relative time calculations.  (To do this,
> I've had to supply an additional clock_type template parameter.) 
> 
>       template<typename clock_type, typename duration_type>
>       bool condition_variable::wait_for(unique_lock<mutex>& m, 
>                   const clock_type &clock, const duration_type &rel_time)
>       {
>           return wait_until(m, clock_type::now() + rel_time);
>       }
> 
>       bool condition_variable::wait_until(unique_lock<mutex>& m,
>                   chrono::monotonic_clock::time_point const& wait_until)
> 
>       bool condition_variable::wait_until(unique_lock<mutex>& m,
>                   chrono::system_clock::time_point const& wait_until)
> 
> Critically, this arrangement honours the required clock type, and allows the
> time parameter to be passed through to pthread_cond_timedwait(..., &t) 
> unmolested.

I do not believe templating wait_for on clock_type is a good idea as a monotonic clock (or the closest thing a platform has to it) should always be used for the *_for API.

> While the boost::condition_variable API remained ignorant of clocks other than
> "the system clock" this problem was just an inconvienience.  However, now that
> condition variables have acquired the explicit capability to reference the POSIX
> CLOCK_MONOTONIC (through the monotonic_clock and time_point<monotonic_clock>
> classes), the API is _actively_ misleading.
> 
> As for a solution, I think boost::condition_variables should become
> parameterised on the clock type - mixing 2 time bases in a single condition
> variable is broken. 
> 
> 	template<clock_type>
> 	class condition_variable_clocked;
>        // backwards compatibility
>        typedef boost::condition_variable<system_clock> condition_variable;
> 
>        // create a condvar that uses POSIX CLOCK_MONOTONIC
> 	boost::condition_variable<chrono::monotonic_clock> cv1; 
> 
>        // create a condvar that uses POSIX CLOCK_REALTIME
> 	boost::condition_variable<chrono::system_clock> cv2;    
> 
> 
> I'd appreciate any comments on the above.

Templating the condition_variable on clock_type was specifically avoided for std::condition_variable because a condition_variable often has more than one waiting client at a time, and it was felt that those different waiting clients should be able to use different clocks simultaneously when waiting on a (absolute) time_point.  For example one client may wish to time out at 7pm tonight as measured by the system clock, while at the same time another client may wish to time out 200 milliseconds from the time it entered the start of the current function as measured by the monotonic clock.  Such flexibility would not be possible if the clock type was encoded into the condition variable type.

That being said, there is absolutely nothing wrong with creating a condition_variable_clocked<clock_type> that wraps condition_variable and imposes a specific clock, or family of clocks on the client.  This might give needed control to the author of some type that needed to expose a cv-like object in its API.  Such a cv-adaptor did not seem appropriate for standardization, but that doesn't mean it wouldn't be a good component of boost.

Note that std::condition_variable_any ([thread.condition.condvarany]) is nothing more than a cv-adaptor.

-Howard