Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW

Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2010-11-11 04:54:48

----- Original Message ----- 
From: "John Bytheway" <jbytheway+boost_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Wednesday, November 10, 2010 8:40 PM
Subject: Re: [boost] [Review] Formal Review of Proposed Boost.Chrono Library Starts TOMORROW

> On 10/11/10 00:04, vicente.botet wrote:
>> Thanks John for your contribution. ----- Original Message ----- From:
>> "John Bytheway" <jbytheway+boost_at_[hidden]> To:
>>> On 05/11/10 17:25, Anthony Williams wrote:
>>>> Here are some questions you might want to answer in your review:
>>>> 
>>>> - What is your evaluation of the design?
>>> 
>>> It looks mostly well thought out and sensible, and I guess there's
>>> not much to say about the standardized portion, but I have a few
>>> concerns:
> 
>>> There is various compile-time arithmetic happening implicitly in
>>> this library (e.g. in the many common_type return values).  Are
>>> integer overflows in this arithmetic detected and reported?  I
>>> think doing so would be valuable.
>> 
>> The boost ratio library avoids all kind of overflow that coudl
>> resulting of arithmetic operation and can be simplified. The typedefs
>> durations don't detect overflow. You will need a representation that
>> handles with. I don't think the library should change the standard
>> behavior, but could use representation the application gives for
>> debug. Would this responds to your needs?
> 
> Sorry, I don't follow.  I think you're talking about runtime arithmetic
> here; I'm talking about compile-time arithmetic.  

I wasn't clear about what overflow you were talking so for ratio the answers is yes.

> For example
> 
> uint64_t const big = uint64_t(1)<<62;
> typedef duration<uint64_t, ratio<1, big>> D1;
> typedef duration<uint64_t, ratio<1, big-1>> D2;
> D1 d1(0);
> D2 d2(0);
> auto d = d1+d2;
>
> Will the above compile?  > If so, what is the type of d?

No. there is an overflow. The following compile-time error is obtained:
 
> This is really a ratio library question; perhaps if I read the docs
> thereof it would tell me.  But I'm concerned because of all the implicit
> combination of ratios in Boost.Chrono.

I understand.
 
>>> This function puzzles me:
>>> 
>>> template <class Rep1, class Rep2, class Period> double
>>> operator/(const Rep1& s, const  duration<Rep2, Period>& d);
>> 
>> This overloading is used to get the frequency. I added it because I
>> needed it for Boost.Stopwatch which report could report the frequency
>> of an event.
> 
> OK, that makes sense.  I can see why it's useful; it just makes me a
> little uneasy because the units of the return value depend on the units
> of d in a way that most of the operations strive to avoid.
> 
> If you added it for Boost.Stopwatch do I take it that it's not in the
> standard proposal?

Yes :(.
 
>>> What is its intended purpose?  It seems to violate the type system 
>>> somewhat by returning a double rather than an 'inverse duration'.
>> 
>> What would you expect as result?
> 
> Unless you returned something from Boost.Units there really isn't a
> sensible option (and I'm certainly not suggesting you return something
> from Boost.Units).

You are right. Whithout the correct units the result has no sense. For coherency, I think that I should be forced to remove this overloading, and let Stopwatch make his internal computation.

>>> You seem to be guaranteeing that high_resolution_clock is a typedef
>>> of one of the other two clocks?  Wouldn't it be better to leave
>>> open the possibility that it uses some other, more high-resolution
>>> clock?
>> 
>> Yes I could. I don't see nothing in the standard that forbids it. Do
>> you have a better implementation for high-resolution_clock  and for
>> which platform? I could include it if you have one.
> 
> For Windows there's QueryPerformanceCounter and
> QueryPerformanceFrequency that people are always suggesting for use in
> Boost.Timer. 

Boost.chrono uses already them. I will add the following table as requested.

"The following table presents a resume of which API is uused for each clock on each platform
[table Clock API correspondence
    [[Clock]                                   [Windows Platform]                         [Posix Platform]        [Mac Platform]  ]
    [[__system_clock__]              [GetSystemTimeAsFileTime]          [clock_gettime( CLOCK_REALTIME)] [gettimeofday]          ]
    [[__monotonic_clock__]       [QueryPerformanceCounter and QueryPerformanceFrequency]          [clock_gettime( CLOCK_MONOTONIC)] [mach_timebase_info,mach_absolute_time]          ]
    [[__process_*_cpu_clock__] [GetProcessTimes]                          [times]        [times]  ]
    [[__thread_clock__]               [GetThreadTimes]                           [pthread_getcpuclockid]   [pthread_getcpuclockid (Not tested)]       ]
]
" 

> I've never used them, but I get the impression they're
> higher precision.  OTOH, I think that their precision is not known at
> compile-time, which may be troublesome.

The way to manage with precission at run-time, is to state an desired precission at compile-time and convert at run-time. This means that if the run-time resolution is higher than the compile-time one precission is lost. The current implementation for windows set the Clock duration unit to nanoseconds. 

> I believe there are similar things on other platforms.  One thing I
> would like is a common interface akin to libfftw's getticks.  (See the
> "Cycle Counters" part of http://www.fftw.org/download.html).  This is
> also in arbitrary units, not seconds, though, and I'm not sure how many
> of the underlying APIs have a known compile-time precision.

Without the processor speed the getticks() function is not of utility for the Chrono framework. If the user know the processor speed in Mhz at compile-time s/he can define a cycle_count_clock in a simple way as follows:

template <long long speed>
struct cycle_count_clock
{
  typedef typename boost::ratio_multiply<boost::ratio<speed>, boost::mega>::type frequency; // Mhz
  typedef typename boost::ratio_divide<boost::ratio<1>, frequency>::type period;
  typedef long long rep;
  typedef boost::chrono::duration<rep, period> duration;
  typedef boost::chrono::time_point<cycle_count_clock> time_point;
  static time_point now()
  {
    // return exact cycle count
    return time_point(duration(::getticks())); // access to clock cycle count
  }
};

and use it as follows on a 1600 MHz processor:

typedef cycle_count_clock<1600> clock;

There is an example that uses an emmulated getticks and uses this kind of clock.

> So perhaps all these things do not belong in Boost.Chrono.
>
>>>> - What is your evaluation of the documentation?
>>> As others have said, I would appreciate a description of what the 
>>> different clocks are.  In particular, I've always found 'monotonic'
>>> a bizarre adjective in this context; in what way is it more
>>> monotonic than the others?  (I think the answer is buried in the
>>> reference, under the definition of Clock::is_monotonic).
>> 
>> I have used the name of the standard. But maybe the name will change
>> to steady_clock.
> 
> Are you saying the standardised name might change?

The standard is no finished yet (Mars 2011?). So yes, the standard can change. It is not thatthey will change the name of the class but add a new one with a different semantic and remove the monotonic_clock. This doesn't means that Boost.Chrono will break compatibility if the standard change. I will just add the new clock.

>>> I think it would be worth mentioning (probably in the FAQ)
>>> something about benchmarking, since that is a common requirement
>>> leading people to want access to clocks.  For example it could say
>>> which, if any, of the provided clocks are appropriate for
>>> benchmarking, and what to watch out for.
>> 
>> Each clock has his own features. It depends on what do you need to
>> benchmark. Most of the time, you could be interested in using a
>> thread clock, but if you need to measure code subject to
>> synchronization a process clock would be better. If you have a
>> multi-process application, a system-wide clock will be needed.
> 
> Yes, that kind of information is worth putting n the FAQ, I think.

Added.
 
>>> Some time ago Edward Grace proposed on this list some well thought 
>>> out benchmarking techniques 
>>> <http://article.gmane.org/gmane.comp.lib.boost.devel/191711>.  It
>>> would be nice if that or something like it found more prominence
>>> (but Boost.Chrono is not the place for that).
>> 
>> There is Boost.Stopwatch which will be reviewed 1Q/2Q 2011 that
>> should cover the needs. Please take a look at and let me know if you
>> fid there what you are looking for.
> 
> I tried, but the documentation link on LibrariesUnderConstruction
> doesn't seem to work (I get a blank page).

I don't get  blank page. Please could you try http://svn.boost.org/svn/boost/sandbox/chrono/libs/stopwatches/doc/html/index.html
 
>>>> And finally, every review should answer this question:
>>>> 
>>>> - Do you think the library should be accepted as a Boost
>>>> library?
>>> 
>>> Yes, but the docs need more work (both on the high level and in
>>> detail). All the reviewers have commented on this so far, and I
>>> recommend that when the documentation is revised it should be
>>> offered for comment again (preferably to be examined by people who
>>> didn't look at it this time :)).
>> 
>> Thanks a lot. I appreciate all your comment and expect they will help
>> me to improve the library documentation and usability.
> 
> You're welcome :).
> 
>> I will do it of course, but I think that it will be diffcult to find
>> other volunters, so it will be great if you could review it again
>> :).
> 
> I doubt I could as well a job on a second reading, but perhaps I could
> try...

Thanks anyway,
Vicente