-
-
Notifications
You must be signed in to change notification settings - Fork 747
Add the MonoTime equivalents of std.datetime.StopWatch/benchmark. #5367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
FYI: I've already talked about these changes with Andrei, and he's okay with them. |
wilzbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned offline I think this is an excellent opportunity to change benchmark, s.t. it will avoid optimization the benchmarked functions away. Though, this could be easily done as a follow-up as well.
Moreover, I added a couple of nits & questions.
| If $(D StopWatch.init) is used, then the constructed StopWatch isn't | ||
| running (and can't be, since no constructor ran). | ||
| +/ | ||
| this(AutoStart autostart) @safe nothrow @nogc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this has been discussed when StopWatch has been introduced, but just out of interest is setting a default being avoid to force the user to be explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from whether that would help or harm usability, it wouldn't work in D, because then it would be a default constructor.
| { | ||
| sw.reset(); | ||
| foreach (_; 0 .. n) | ||
| fun[i](); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor thing that one nearly always has to think about is that a reasonably smart optimizer will just remove this code block.
There are a couple of tricks to avoid this behavior, at least:
- assign return value to
__gshared - use the
doNotOptimizeAwaytrick (see below or Parameterized unittests and benchmarks #2995) @weakin LDC- ...
Imho it would make benchmark a lot easier to use if this would be done automatically.
FWIW Andrei mentioned once:
but doesn't include things like "doNotOptimizeAway" which is essential.
private void doNotOptimizeAway(T)(auto ref T t)
{
import core.thread : getpid;
import std.stdio : writeln;
if(getpid() == 1) {
writeln(*cast(char*)&t);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to leave that improvement to a separate PR. The focus of this is to give us what we already have but using MonoTime and Duration rather than TickDuration.
std/datetime/stopwatch.d
Outdated
| /++ | ||
| Module containing some basic benchmarking and timing functionality. | ||
|
|
||
| For convenience, this module publicly imports core.time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: For consistency core.time could be a MREF...
std/datetime/stopwatch.d
Outdated
| start should not be called if the StopWatch is already running. | ||
| +/ | ||
| void start() @safe nothrow @nogc | ||
| in { assert(!running); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a longer discussion this summer on which a majority agreed that an assert statement should have a message even if it's trivial as it greatly helps the user in figuring out what went wrong instead of the bare segfault.
std/datetime/stopwatch.d
Outdated
| stop should not be called if the StopWatch is not running. | ||
| +/ | ||
| void stop() @safe nothrow @nogc | ||
| in { assert(_running); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about assert with a error message
| result[i] = sw.peek(); | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask what the rationale for removing opEquals was?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably removed it with the idea that it did the same thing as the defaults, since when StopWatch was originally added, it was not uncommon that you had to declare it yourself. Looking over it now, it looks like the original implementation is different from the default in that it skips comparing the _running member, therefore not treating whether the StopWatch is running as part of its comparable state. I don't know if that makes sense or not. I strongly question whether it makes any sense to compare a StopWatch in the first place.
std.datetime.package has StopWatch, benchmark, comparingBenchmark, and measureTime, all of which use TickDuration (which would be deprecated, but it can't be deprecated as long as those functions in std.datetime are deprecated). This commit introduces std.datetime.stopwatch to replace those functions in std.datetime. In order to avoid symbol conflicts, std.datetime.stopwatch will not be publicly import in std.datetime.package until the old symbols have been removed. std.datetime.experimental.stopwatch contains StopWatch and benchmark which have essentially the same APIs as the ones in std.datetime.package, but they use MonoTime and Duration. comparingBenchmark has not been ported to MonoTime and Duration, because it is simply a wrapper around benchmark. measureTime has not been ported to MonoTime and Duration, because it is equivalent to using StopWatch with a scope(exit) statement. The old functionality will be deprecated the major release after the new symbols have been introduced.
Now, it includes information on std.datetime.stopwatch.
|
Updated based on Sebastian's comments. |
wilzbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for doing this work @jmdavis!
std.datetime.package has StopWatch, benchmark, comparingBenchmark, and
measureTime, all of which use TickDuration (which would be deprecated,
but it can't be deprecated as long as those functions in std.datetime
are deprecated). This commit introduces
std.datetime.stopwatch to replace those functions in std.datetime. In
order to avoid symbol conflicts, std.datetime.stopwatch will not be
publicly import in std.datetime.package until the old symbols have been
removed.
std.datetime.experimental.stopwatch contains StopWatch and benchmark
which have essentially the same APIs as the ones in
std.datetime.package, but they use MonoTime and Duration.
comparingBenchmark has not been ported to MonoTime and Duration, because
it is simply a wrapper around benchmark.
measureTime has not been ported to MonoTime and Duration, because it is
equivalent to using StopWatch with a scope(exit) statement.
The old functionality will be deprecated the major release after the new
symbols have been introduced.