On Mon, Sep 19, 2011 at 4:39 PM, Stewart, Robert <Robert.Stewart@sig.com>wrote:
Beman Dawes wrote:
http://beman.github.com/timer/ documents a useful replacement for Boost.Timer. The full library has been in the sandbox for years, but current development is on GitHub.
You use "_t" for a typedef and a struct. Odd.
Historical artifact. typedef changed to nanosecond_type. struct changed to cpu_times. See later response to naming questions for rationale.
Why doesn't times() return a times_t? I realize times_t comprises three 64b values, but do you suppose that function will be called in high performance contexts in which returning 192 bits would be problematic?
Interesting question. I don't see a performance concern, and the interface would be cleaner if the result is just returned. The issue is actually moot for times() because times() has been eliminated.It makes much more sense in a C++11 world to implement in terms of std <chrono> as implemented by Boost.Chrono, and that has been done and is working well.
I may be unobservant, but this is the first time I've noticed a function taking *and* returning a system::error_code &. Is that beneficial? (If that's an idiom you'd encourage, it should be documented in Boost.System.)
No, that's a holdover from years ago when I was experimenting with different ways to use system::error_code. Changing the one-argument times() to return a times_t would mean this one,
too, should return a times_t, but in the case of error, would it be a copy of an uninitialized instance? Maybe the times_t & argument isn't so bad. :)
There is also the C convention of returning times of -1 on an error. See destructor discussion below.
timer::stop() is inconsistent. If times() and timer::elapsed() take a times_t & argument, then so should timer::stop().
I'll make it consistent, probably by returning the result rather than taking an argument.
s/stopped/is_stopped/
Changed.
I dislike the idea of relying on a destructor to report timing information as you've done in run_timer. The no-throw demands of a destructor make I/O questionable since there is no means to report failure. I realize that there is a report() member function that can be called separately, but using the destructor just seems too odd.
It comes down to ease of use. In practice, the idiom "just works".
"run_timer" doesn't indicating the principle feature: reporting. You might call it "reporting_timer" or "reportable_timer"?
"report" does not imply a call to stop().
I've also been concerned about names. Some background: * The resolution of timer/run_timer was distressingly low on Windows/Linux/Mac platforms. As low as 1/10 of a second, and never much better. * Wall clock-time (AKA real-time) could be improved to sub-microsecond resolution by using platform specific high resolution APIs, as Boost.Chrono was already doing. * Tests showed that use of high resolution timers for wall clock time was a big step forward, but the cost of always getting user and system time, even if not used, was quite noticeable, particularly with GCC. The response to that was to provided two timers, one just reporting wall clock time, and the other reporting wall clock, user, and system time. What should these be named? I settled on: class high_resolution_timer; // wall-clock time class auto_high_resolution_timer; // wall-clock time, with reporting class cpu_timer; // wall-clock, user, and system time class auto_cpu_timer; // wall-clock, user, and system time, with reporting Applying your suggestion yields reporting_high_resolution_timer and reporting_cpu_timer. reporting_high_resolution_timer is kinda long, so we might use hi_res_timer and reporting_hi_res_timer. I'm OK with any of those, but have a mild preference for auto_ rather than reporting_. I waver back and forth on high_resolution vs hi_res.
Because of all of those strikes against run_timer, I'd prefer something like this:
class timer { public: // as before
void report(int _places = 2); // writes to std::cout
void report(int _places, std::ostream & _stream);
void report(std::string const & _format, int _places = 2);
void report(std::string const & _format, int _places, std::ostream & _stream); };
Those member functions would capture a snapshot of the elapsed time and report it. (That means the I/O would affect further timing values; an accumulated elapsed time and a current start time could be used to ignore the time required for I/O.) A call to stop() followed by a call to report() would be needed to report the elapsed time.
It still might be useful to provide an RAII class to ensure a timer stops at the end of a scope:
timer t; { stopper _(t); // do stuff }
Interesting, but mixing timing and reporting in the same class doesn't seem desirable to me. My most frequent use of timers has been for reporting, and I really want to keep that usage simple. The difference between: int main() { timer t; { stopper _(t); // do stuff } } and: int main() { auto_timer t; // do stuff } is really major and would make the usage harder to teach and less likely to be used, IMO. Thanks for these comments! Very helpful. --Beman