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: John Bytheway (jbytheway+boost_at_[hidden])
Date: 2010-11-10 14:40:20

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.  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?

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.

>> 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?

>> 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 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 forbid 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.  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.

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.

So perhaps all these things do not belong in Boost.Chrono.

>>> - What is your evaluation of the documentation?
>> 
>> The docs say that the code fragments have an implicit "using
>> namespace boost::chrono;".  In fact in practice many of them use
>> the namespace explicitly or include "using namespace chrono;".
>> Could you use a namespace alias so that it is clear exactly which
>> names are taken from this namespace.
> 
> This will take some time and will make the code and the documentation
> more weigth but I understand your concern, as it allows to see
> clearly what is part of the library. I will do it if other think it
> is better for the library.

That's reasonable :).

>> 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?

> A monotonic clock is a clock for which values of
> time_point never decrease as physical time advances as steady clock
> represent clocks for which values of time_point advance at a steady
> rate relative to real time. That is, the clock may not be adjusted.
> 
> The system_clock could be adjusted to a time_point that decresase the
> last time_point so is is not monotonic.
> 
> I will add the distinction in the documentation.

Thanks.  It puzzled me for years :).

>> Along similar lines, I think that somewhere (perhaps the FAQ or
>> the rationale) should explain which APIs have been chosen to
>> implement each clock on each platform, and why.
> 
> I could add something to the Implementation Notes section.

:)

>> If there are any popular APIs for which clocks have not been
>> written, are there plans to do so in the future?
> 
> I'm not aware of all the popular clocks API, but I don't know other
> than the provided by the library.

Cool.

>> 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.

>> 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).

>>> 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...

John Bytheway