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

[Greg Chicares]

On 6/3/21 7:52 PM, Vadim Zeitlin wrote:
> On Thu, 3 Jun 2021 19:14:20 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:

[...snip discussion of the standard date class's shortcomings...]

>  But, more importantly, we're speaking just about clocks here, not dates,
> so this is completely unrelated to the subject at hand.

Okay, let's consider using only the standard clock stuff in isolation.

> GC> If temporary convenience in one source file you're working on is
> GC> the motivation, then would something as simple as
> GC> 
> GC> +#include "timer.hpp"
> GC> +#include "timer.cpp"
> GC> 
> GC> be enough?
> 
>  This would make compiling this file even longer and, well, just seems
> ugly. But currently I insert
> 
>       #include "stop_watch.hpp"
> 
> with my own code anyhow, so I guess it's not that different.

But what's the problem that you want to address?

(1) Typing two '#include' directives when it seems that one should do?
In that case, we could create a single "timer_inline.hpp" header that
would consist of some boilerplate plus those two directives.

(2) Compiling all the code in 'timer.cpp' whenever it's included as above?
Can that really take much time? Just now, I loaded it in vim and and
removed all the conditional stuff, leaving only a posix implementation;
then I removed the verbose precondition tests; and then, counting only the
function bodies inside {}, only twenty-five lines of code remain. Including
<chrono> must bring in even more code and make compilation even slower.

(3) The mere existence of the platform conditionals is objectionable. We
could separate it into two files, 'timer_posix.cpp' and 'timer_msw.cpp',
but that only moves the conditionals around rather than eliminating them.

I think (2) doesn't stand up to analysis, so the questions are
  (A) Does 'timer.?pp' want to be a header-only thing?
  (B) Should the relevant subset of std::chrono replace the
      platform-conditional code?

Then (A) is a matter of taste AFAICS: I haven't analyzed it deeply,
but I don't see why this couldn't be turned into a standalone header,
though I'm not sure it should be. And (B) is a substantive question
that can be resolved by rational consideration.

> GC> What we have is familiar and well validated; why replace it?
> 
>  To get rid of ugly-looking platform-specific code, possibly making it
> portable to more platforms in the future. And just to reduce the line count
> which is IMO always a good thing.

OK, that's (B) above. As discussed in (2) above, (B) dramatically
reduces the line count.

Your 'stop_watch.hpp' example would be a further simplification:
it removes stop(), start(), and restart(). But restart() is just
    elapsed_time_ = 0;
    start();
    return *this;
so I don't imagine that removing it is crucial to your proposal.
Similarly, I don't think you'd object to this one-liner:
    void lmi_sleep(int seconds) {sleep(bourn_cast<unsigned int>(seconds));}
so I think this discussion concerns mostly (B).

> GC> Perhaps using a tiny part like std::chrono::high_resolution_clock
> GC> would be a good idea for the long term, as it would automatically
> GC> adapt to operating-system changes.
> 
>  Yes.

+1 for (B). Maybe I should say +5, because I have no real wish
to maintain that msw-specific stuff.

> GC> That might even make sense in the short term, if we can verify that the
> GC> bindings to glibc and msw are correct--for glibc, I don't think we'd
> GC> even have to ask, but for MinGW-w64, it's important to ask.
> 
>  Yes, verifying this is part of what I planned to do.

Thanks. I'll score that point as
  -10 until it's confirmed, but
  +0 once it's confirmed.

> GC> Oh, but wait:
> GC> 
> GC>   https://en.cppreference.com/w/cpp/chrono/high_resolution_clock
> GC> | The high_resolution_clock is not implemented consistently across
> GC> | different standard library implementations, and its use should be
> GC> | avoided.
> GC> 
> GC> Of course, that's not normative, but it seems scary: it sounds
> GC> like if I decide I want a high-resolution clock (which I can
> GC> implement very well myself), but some implementation decides to
> GC> veto my decision, then they prevail. Shouldn't I just veto them
> GC> in advance by avoiding std::chrono?
> 
>  No, I don't think so because I don't know of any platform that does this
> and I think it's rather unlikely that any of the currently supported ones
> would ever do it.

Okay. Back when the boost version was being designed, it offered
only a low-resolution clock; here's the discussion as I remember it:
 -- But I really, really want a high-resolution clock.
 -- No, you're wrong, you shouldn't want that. [snip technical reasons]
 -- But I do! [snip discussion showing full understanding of the issues]
 -- No. We're right, and you're wrong, because we say so.

Score:
 - 100 if that kind of attitude didn't get smuggled into the standard;
 + 0 upon confirmation that it didn't: IOW, if all of
     {gcc,clang} X {pc-linux-gnu,*msw} provide a
     genuine high-resolution timer.

Total:
  +5 +0 +0 = +5 if your research confirms that my fears aren't realized
  -5 or -95 otherwise
and the final scoring is now in your hands.

> GC> And if we use std::chrono for this, what do we get when we ask
> GC> for the tick count? A 64-bit integer? No: std::chrono::time_point,
> GC> with a helper class std::common_type<std::chrono::time_point> and
> GC> so on. Why isn't that wrong? Why shouldn't it be as simple as an
> GC> integral tick count? I worry that using any part of std::chrono
> GC> introduces a taint that would tend to propagate.
> 
>  I admit that I also find time_point and durations over-generic, but this
> might be appropriate for the standard library which is supposed to be used
> in many different circumstances. Of course, in your particular
> circumstances you don't need to use all of these possibilities and just
> need to select whether you want to use an integer or a floating-type for
> the ticks representation and the scaling you want to use.

Okay, as long as we can encapsulate all of this in a Timer class.

>  E.g. in my stop_watch.hpp I just do the equivalent of the following:
> 
>     using clock = std::chrono::high_resolution_clock;
>     clock::time_point const start = clock::now();
> 
>     ... code to benchmark here ...
> 
>     clock::duration const elapsed = clock::now() - start;
>     std::cerr
>         << "Elapsed time: "
>         << std::setw(10)
>         << 
> std::chrono::duration_cast<std::chrono::microseconds>(elapsed).count()
>         << "us\n"
>         ;
> 
> which is really not that bad and definitely much simpler than the
> equivalent code in timer.cpp.

Yes and no. No platform conditionals in 'timer.cpp': good. OTOH,
selecting an example or two from `git grep elapsed_ *.?pp`...

    status() << "Delete: " << timer.stop().elapsed_msec_str() << std::flush;

That seems preferable to writing it out inline:

    status() << "Delete: " << std::setw(10) << 
std::chrono::duration_cast<std::chrono::microseconds>(clock::now() - 
start).count() << std::flush;

which is my point above about encapsulating everything in class Timer.

  double ledger_emitter::emit_cell( ...
  {
    Timer timer;
    ...
    return timer.stop().elapsed_seconds();
  }

Here I'd rather write
    Timer timer;
than
    using clock = std::chrono::high_resolution_clock;
    clock::time_point const start = clock::now();

and return 'double' rather than something complicated--i.e., I don't
ever want to write anything like this:

  std::chrono::duration_cast<std::chrono::microseconds>

anywhere except in a Timer class implementation (and even there,
preferably only once). I think this argues in favor of separate
compilation instead of a header-only implementation.