Require a steady clock for bench with at least micro precision

TheBlueMatt commented at 4:49 pm on November 9, 2017: member

Using a non-steady high_precision_clock by default is definitely not what we want, and in practice steady_clock has more than enough precision. Should double-check that travis passes on this one to make sure we actually have at least microsecond precision on all platforms.

laanwj commented at 4:53 pm on November 9, 2017: member

Concept ACK

MarcoFalke commented at 5:02 pm on November 9, 2017: member

utACK bb3f030c5559a5053c45cbd0bfebdc3f26703033. (They have the same precision on my system, anyway)

in src/bench/bench.h:46 in bb3f030c55 outdated

45         using hi_res_clock = std::chrono::high_resolution_clock;
46         using steady_clock = std::chrono::steady_clock;
47-        static constexpr bool steady_is_high_res = std::ratio_less_equal<steady_clock::period, hi_res_clock::period>::value;
48-        using type = std::conditional<steady_is_high_res, steady_clock, hi_res_clock>::type;
49+        using type = std::conditional<hi_res_clock::is_steady, hi_res_clock, steady_clock>::type;
50+        static_assert(std::ratio_less_equal<type::period, std::micro>::value, "steady_clock must have at least microsecond precision");

laanwj commented at 5:07 pm on November 9, 2017:

Thinking about it, I’m not sure I like the static assert here - do we really prefer the benchmarks to not compile to them compiling with possibly lower precision? Normally a runtime warning would be enough for these kind of things.

TheBlueMatt commented at 5:29 pm on November 9, 2017:

I was figuring that since its (AFAIK) always the case that the steady_clock has microsecond precision it wasnt a big deal….would use a static_warning if possible, but I guess I could do a runtime print if we prefer that.

laanwj commented at 5:59 pm on November 9, 2017:

Don’t care whether it’s a runtime or compile-time warning. It’s just that I already predict complaints coming from someone building on some obscure platform that has lower precision, who probably doesn’t even care about the benchmarks but just that the project doesn’t build.

TheBlueMatt commented at 6:06 pm on November 9, 2017:

OK, I moved it to a runtime print.

TheBlueMatt force-pushed on Nov 9, 2017

theuni commented at 6:13 pm on November 9, 2017: member

Sure, I went back and forth on which to prefer. In practice, I suspect they’ll usually be the same anyway.

As a data point, while testing libstdc++/libc++/mingw, linux and osx, The only (compile-time) difference I observed between clocks was with libc++. It was the same for osx/linux:

0high res precision: nanosecond
1steady precision: nanosecond
2system precision: microsecond

Everywhere else was nano for all 3.

TheBlueMatt commented at 6:20 pm on November 9, 2017: member

I don’t believe they can be the same - it seems like most non-windows platforms have a non-steady high_precision_clock, meaning you will get different results there…

On November 9, 2017 1:13:12 PM EST, Cory Fields notifications@github.com wrote:

Sure, I went back and forth on which to prefer. In practice, I suspect they’ll usually be the same anyway.

As a data point, while testing libstdc++/libc++/mingw, linux and osx, The only (compile-time) difference I observed between clocks was with libc++. It was the same for osx/linux:
0high res precision: nanosecond
1steady precision: nanosecond
2system precision: microsecond
Everywhere else was nano for all 3.

– You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11646#issuecomment-343242338

in src/bench/bench.cpp:27 in ad0d1e14e3 outdated

22@@ -23,6 +23,9 @@ void
23 benchmark::BenchRunner::RunAll(benchmark::duration elapsedTimeForOne)
24 {
25     perf_init();
26+    if (std::ratio_less_equal<benchmark::clock::period, std::micro>::value) {
27+        std::cout << "WARNING: Clock precision is worse than microsecond - benchmarks may be less accurate!\n";

theuni commented at 6:20 pm on November 9, 2017:

Print to stderr to avoid breaking csv parsing?

Require a steady clock for bench with at least micro precision 620bae34cf

in src/bench/bench.h:41 in ad0d1e14e3 outdated

36@@ -37,13 +37,12 @@ BENCHMARK(CODE_TO_TIME);
37  */
38  
39 namespace benchmark {
40-    // On many systems, the high_resolution_clock offers no better resolution than the steady_clock.
41-    // If that's the case, prefer the steady_clock.
42+    // In case high_resolution_clock is steady, prefer that, otherwise use steady_clock.
43+    // ...but always require at least microsecond precision!

theuni commented at 6:28 pm on November 9, 2017:

comment is stale now

TheBlueMatt force-pushed on Nov 9, 2017

theuni commented at 10:32 pm on November 9, 2017: member

utACK 620bae34cfe10e20daa0dcec7e4b9ffee8dfd397

fanquake added the label Tests on Nov 9, 2017

laanwj commented at 7:21 am on November 10, 2017: member

utACK 620bae3

laanwj merged this on Nov 10, 2017

laanwj closed this on Nov 10, 2017

laanwj referenced this in commit fe503e118f on Nov 10, 2017

laanwj commented at 7:24 am on November 10, 2017: member

This is kind of amusing; seems the person that introduced high_resolution_clock in the first place, Howard Hinnant, has second thought about it:

I would not be opposed to deprecating high_resolution_clock, with the intent to remove it after a suitable period of deprecation. The reality is that it is always a typedef to either steady_clock or system_clock, and the programmer is better off choosing one of those two and know what he’s getting, than choose high_resolution_clock and get some other clock by a roll of the dice. - https://stackoverflow.com/questions/38252022/does-standard-c11-guarantee-that-high-resolution-clock-measure-real-time-non

So yes, this is a good choice. If we want a steady clock, use a steady clock.

in src/bench/bench.cpp:26 in 620bae34cf

22@@ -23,6 +23,9 @@ void
23 benchmark::BenchRunner::RunAll(benchmark::duration elapsedTimeForOne)
24 {
25     perf_init();
26+    if (std::ratio_less_equal<benchmark::clock::period, std::micro>::value) {

MarcoFalke commented at 8:54 pm on November 13, 2017:

I assume you meant to invert that condition, i.e. print the warning when our period is not less_equal micro-precision.

MarcoFalke added the label Up for grabs on Nov 13, 2017

laanwj removed the label Up for grabs on Nov 15, 2017

laanwj added the label Up for grabs on Nov 15, 2017

MarcoFalke removed the label Up for grabs on Nov 27, 2017

zkbot referenced this in commit aa225ebb0b on Jan 24, 2020

zkbot referenced this in commit 74ff73abab on Jan 24, 2020

PastaPastaPasta referenced this in commit d660016f21 on Jan 31, 2020

PastaPastaPasta referenced this in commit 719414d034 on Feb 4, 2020

PastaPastaPasta referenced this in commit 523ecf915b on Feb 9, 2020

furszy referenced this in commit 4ed15cc69d on Jun 8, 2020

ckti referenced this in commit a6a21df795 on Mar 28, 2021

gades referenced this in commit 576844f33f on Jun 28, 2021

DrahtBot locked this on Sep 8, 2021

Require a steady clock for bench with at least micro precision #11646