Re: [lmi] Using std::chrono::high_resolution_clock for Timer implementation

[Vadim Zeitlin]

On Thu, 3 Jun 2021 22:37:47 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> On 6/3/21 7:52 PM, Vadim Zeitlin wrote:
GC> > On Thu, 3 Jun 2021 19:14:20 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:
GC> 
GC> [...snip discussion of the standard date class's shortcomings...]
GC> 
GC> >  But, more importantly, we're speaking just about clocks here, not dates,
GC> > so this is completely unrelated to the subject at hand.
GC> 
GC> Okay, let's consider using only the standard clock stuff in isolation.
[...]
GC> But what's the problem that you want to address?

 Sorry, I should have been more clear about this, so let me try to do it
now, following my usual better-late-than-never motto:

1. There is no real problem per se here, i.e. this is not critical at all.
2. There is a potentially nice improvement consisting in replacing
   platform-specific code in lmi with the code using the standard library.
3. There is another, smaller, potential improvement consisting in making
   Timer class fully inline, which depends on the previous point.

 I thought the point (2) above was so obviously an improvement that it
didn't need a rationale, and after reading your reply, I believe that you
do actually agree with it, so I won't waste your time by discussing this
further, but please let me know if I misunderstood you and if you're still
not convinced that it's actually worth doing.


GC> I think (2) doesn't stand up to analysis, so the questions are
GC>   (A) Does 'timer.?pp' want to be a header-only thing?
GC>   (B) Should the relevant subset of std::chrono replace the
GC>       platform-conditional code?
GC> 
GC> Then (A) is a matter of taste AFAICS: I haven't analyzed it deeply,
GC> but I don't see why this couldn't be turned into a standalone header,
GC> though I'm not sure it should be. And (B) is a substantive question
GC> that can be resolved by rational consideration.

 I think (A) would become more clear once (B) is done, so I'd rather
address them in the reverse alphabetical order.
 

GC> Your 'stop_watch.hpp' example would be a further simplification:

 Sorry, the example in my email was maximally simplified to show just the
use of <chrono> API and doesn't faithfully represent my actual class (and
yes, I should have been more clear about this too).

GC> >  E.g. in my stop_watch.hpp I just do the equivalent of the following:
GC> > 
GC> >     using clock = std::chrono::high_resolution_clock;
GC> >     clock::time_point const start = clock::now();
GC> > 
GC> >     ... code to benchmark here ...
GC> > 
GC> >     clock::duration const elapsed = clock::now() - start;
GC> >     std::cerr
GC> >         << "Elapsed time: "
GC> >         << std::setw(10)
GC> >         << 
std::chrono::duration_cast<std::chrono::microseconds>(elapsed).count()
GC> >         << "us\n"
GC> >         ;
GC> > 
GC> > which is really not that bad and definitely much simpler than the
GC> > equivalent code in timer.cpp.
GC> 
GC> Yes and no. No platform conditionals in 'timer.cpp': good. OTOH,
GC> selecting an example or two from `git grep elapsed_ *.?pp`...
GC> 
GC>     status() << "Delete: " << timer.stop().elapsed_msec_str() << std::flush;
GC> 
GC> That seems preferable to writing it out inline:
GC> 
GC>     status() << "Delete: " << std::setw(10) << 
std::chrono::duration_cast<std::chrono::microseconds>(clock::now() - 
start).count() << std::flush;
GC> 
GC> which is my point above about encapsulating everything in class Timer.

 Yes, of course, there are no plans to use duration_cast<> outside of this
class. The above was just meant as an illustration of the use of the API,
nothing more. My actual code looks like this:

        {
        static stop_watch sw{"some unique name"};
        stop_watch::runner measure{sw};
        ... code to benchmark here ...
        }

and the "std::cerr << ..." above is in stop_watch dtor because I want to
actually aggregate all the measurements for all calls to this function,
rather than measuring each of them, but please don't pay attention to this
neither, it's a quick and experimental API that I just use for now for
because it's much faster than using perf record/report every time.

GC> I don't ever want to write anything like this:
GC> 
GC>   std::chrono::duration_cast<std::chrono::microseconds>
GC> 
GC> anywhere except in a Timer class implementation (and even there,
GC> preferably only once).

 Yes, absolutely.

GC> I think this argues in favor of separate compilation instead of a
GC> header-only implementation.

 I'm not sure about this, but let me try replacing the contents of
timer.cpp with the code using <chrono> first and then we'll be able to see
this more clearly.

 So, to summarize, I'll confirm that we can rely on getting sufficient
precision from high_resolution_clock on all platforms we're interested in
and, if this is indeed the case, will just do the minimal changes to Timer
class implementation for now (and by "now" I mean "after finishing with
Boost.Regex replacement experimentation"), that won't affect its public
interface. Once this is done, we could discuss making the class fully
inline and, maybe, even some potential enhancements of its API.

 Please let me know if you have any objections to this minimal program,
VZ

pgp6RgxQfuYzR.pgp
Description: PGP signature