Add std.benchmark with benchmarking functions that use MonoTime and Duration

[jmdavis]

Okay. I've been working towards getting rid of TickDuration for a while now, since it's highly confusing (one, it conflates a timestamp of the monotonic clock and a duration of the monotonic clock, and two it means that we have two duration types). MonoTime was added so that we could use it and Duration instead of TickDuration and then remove TickDuration, but the one thing remaining that we have to get rid of in order to be able to get rid of TickDuration is the benchmarking functions in std.datetime. But of course, they need to be replaced with benchmarking functions which use MonoTime and Duration before we can get rid of them.

Unfortunately, because this involves function return types, we can't simply use overloads or anything like that to fix the problem, and the replacement functions need to go into another module so that they don't conflict or force us to come up with worse names. std.benchmark seems like the obvious choice, especially since there was talk of moving them there when Andrei's rejected std.benchmark was reviewed, so this PR creates those functions and puts them in the new module, std.benchmark (which just holds those functions and nothing really new). The std.datetime benchmarking functions can then be deprecated after std.benchmark has been out for a release (so that code that uses the benchmarking functions can build with both master and the previous release, whereas that wouldn't be the case if we deprecated the old stuff at the same time we added the new stuff), and then we can finally deprecate TickDuration in the release after that.

There were four benchmarking items in std.datetime, and this only keeps two. StopWatch and benchmark have been ported over to std.benchmark. However, measureTime and comparingBenchmark have not.

I thought that we'd deprecated measureTime ages ago, but apparently not. It's trivially replaced with a scope statement, and its documentation even shows how. So, I see no reason to keep it - especially when any code using it is going to have to be updated after this PR anyway. And comparingBenchmark is just a wrapper around benchmark that only takes two functions and divides the results to give a ratio. I don't know how valuable that is (it doesn't seem all that useful to me), but it seems like an awful lot of code to keep for what amounts to a simple wrapper (particularly since it declares its own type to hold the results). It just doesn't seem like it adds any real value, it seems like it's pretty trivial for someone to write the wrapper themselves if they really want it. So, I left it out.

But StopWatch and benchmark have been ported over (which was surprisingly painful for StopWatch - mostly because TickDuration is incredibly confusing). So, I don't think that any real functionality has been lost. Their API in std.benchmark is pretty much the same as it was in std.datetime - just with MonoTime and Duration instead, and MonoTime doesn't even make it into the public API. I did consider adding alternate functions to StopWatch that exposed the ticks for the rare case when someone might want that level of precision but decided that it wasn't worth complicating the API and might encourage folks to use the ticks and and shoot themselves in the foot, since it's harder to get that right when you start having to worry about converting clock frequencies and whatnot. Duration should work just fine except in rare cases.

This PR does create a new module, so maybe it should be brought up in the newsgroup or get more of a review than these functions normally would, but it's one function and one type added, both of which already exist in std.datetime - except they use TickDuration there. So, having a full-on module review for it seems like overkill.

[andralex]

All new modules must first go in std.experimental. Thanks!

[jmdavis]

All new modules must first go in std.experimental. Thanks!

If you'd like me to, I certainly can, but I don't see much point in it in this case. We're talking about an update to existing functionality that's just being moved to another module. If there were an existing module where this stuff fit, we wouldn't bother with std.experimental at all. We'd just add the functions.

[DmitryOlshansky]

If there were an existing module where this stuff fit, we wouldn't bother with std.experimental at all. We'd just add the functions.

I guess the only point would be if we really want the name std.benchmark or std.some-package.benchmark.

[jmdavis]

I guess the only point would be if we really want the name std.benchmark or std.some-package.benchmark.

I'm not sure what package we'd put the new module in. std.datetime would be the only candidate that I can think of currently, but it can't got there (even once std.datetime is split), because it would conflict with the existing functions that use TickDuration (and while benchmarking uses time-related stuff, I think that it's different enough from date/time functionality that putting it separately makes more sense anyway - especially if we ever add the kind of functionality that Andrei was trying to add with std.benchmark previously). If someone has a new package name that they think would be really appropriate where we expect additional, related functionality to go, then fine. But otherwise, std.benchmark makes the most sense to me.

An alternative is to give StopWatch and benchmark new names and keep them in std.datetime, but that would be suboptimal from a naming standpoint, since those names are pretty much perfect, and we were already planning on moving them into std.benchmark with Andrei's std.benchmark proposal.

But unless there's something with the API that we've had for these benchmarking functions which needs serious review and consideration, I don't see how putting these functions in an experimental module makes sense simply because I updated them to use MonoTime and Duration. I'm guessing that Andrei just glanced over this and saw that I was adding a new module that wasn't in experimental and thus told me to put it there - which is what we normally do now - and he didn't really read the PR and see what it was adding.

[jmdavis]

@andralex Would you please confirm whether you really want this in std.experimental or not? I'm guessing that you just glanced over this PR, saw that it was adding a new module into std, and told me to put it into std.experimental without really looking at what the PR contained, since while std.experimental makes sense in general for new modules, I don't see how it makes sense in this case. It's just moving existing functionality with a few updates to elsewhere in Phobos, not doing a major redesign or adding new functionality. If the code in question were being moved to an existing module rather than being put in a new one, the idea of putting it in std.experimental would never have even come up.

[CyberShadow]

Unfortunately, because this involves function return types, we can't simply use overloads or anything like that to fix the problem, and the replacement functions need to go into another module so that they don't conflict or force us to come up with worse names.

Programs will still get conflicts if both modules are imported...

[jmdavis]

Programs will still get conflicts if both modules are imported...

Sure, but since std.benchmark doesn't exist yet, only new code will be affected. No code will break due to conflicts. And you don't have to import std.datetime to use std.benchmark. It uses Duration and MonoTime from core.time, not anything from std.datetime. And if code does choose to import both, then it can use the module system to indicate which symbol to use in the short term, and once the old symbols have gone through the deprecation process and been removed, then even that won't be necessary anymore.

But regardless, we can't put the new symbols in std.datetime without giving them new names (which would inevitably be worse, since the current names are pretty much perfect), and when Andrei's std.benchmark was being reviewed previously, the existing versions of these functions were going to be moved into std.benchmark. So, this puts the new symbols where we were already looking to move the old ones, and if we ever do get that expanded benchmark functionality, it can just be merged with the existing benchmarking functions in std.benchmark.

[CyberShadow]

and once the old symbols have gone through the deprecation process and been removed

The problem is that I thought we're not doing that any more, so we'll be stuck with this conflict forever.

[jmdavis]

The problem is that I thought we're not doing that any more, so we'll be stuck with this conflict forever.

There has been talk from time to time about not removing deprecated symbols, but we still do it. We just don't deprecate much anymore, and what does get deprecated usually sticks around for at least two years before it's actually removed. And the undeaD project now exists to hold some of the deprecated stuff permanently for anyone who really wants to use it still (e.g. std.stream and friends was recently deprecated; they've been added to undeaD, and once they've been deprecated for long enough, we're going to remove them from Phobos entirely).

Personally, I think that it's horrible policy to keep deprecated stuff around forever. It's just cruft in the library that slows down the build, impedes maintenance, and risks creating symbol conflicts whenever anyone declares a symbol with the same name - and when there's another symbol with the same name in another module in Phobos, it's that much worse. You're creating a permanent risk of conflicts for pretty much no benefit in the long run. In this case, the replacement symbols are very similar to the existing ones. The required changes to any code using them is minimal, but it is a breaking change, so we need to go through the deprecation process rather than simply changing the existing symbols to use Duration and MonoTime. But once we've put them through the deprecation process and given folks the chance to update their code, leaving the old symbols around will simply cause problems.

[jmdavis]

@andralex I assume that this has just been lost in the sea of stuff that you have to deal with, but we still need your input on whether it's okay to put this functionality directly in std.benchmark or whether we need to put it in std.experimental.benchmark first. It's the same benchmarking functionality that we have in std.datetime now except that it uses MonoTime and Duration instead of TickDuration, so there is no major redesign, and we'd just update the code in-place except that the return types change, so we can't. So, while I agree that putting new modules in std.experimental normally makes sense, I think that it's overkill in this case, and the others commenting on this pull seem to agree.

I'm guessing that you initially told me to put it in std.experimental because you just glanced at it and saw that it introduced a new module without seeing what the pull actually contained, but we obviously can't just veto you and put it in std directly on the assumption that you hadn't seen what was actually in the pull, so we need your input so that we know whether you still think that this needs to go in std.experimental or whether we can merge it into std directly, because it's not really new functionality. Without your input, this pull is stuck in limbo.

Thanks.

[burner]

If you could remove the one benchmark function and rename it to StopWatch you would do me a really big favor, as I have revamping #2995 into a "public" module. Please

[jmdavis]

If you could remove the one benchmark function and rename it to StopWatch you would do me a really big favor, as I have revamping #2995 into a "public" module. Please

I don't follow. We already have benchmark and StopWatch, and it wouldn't make any sense to call a single function stopWatch. A stop watch is an object not a verb. And the name benchmark makes perfect sense for what the function does. These are the names that we've had for years, and they fit perfectly.

What about what you're doing in that PR merits changing these names? The only reason that these functions are changing at all is to get rid of TickDuration. Otherwise, we'd just leave them as-is.

[burner]

my bad I meant to write "benchmark module" not function

[jmdavis]

So, you meant that this should be std.stopwatch rather than std.benchmark? That's not unreasonable, but if you're adding other benchmarking functionality with another PR, then maybe they should either go in the same module ultimately, or maybe they should go in a std.benchmark package in separate modules.

[burner]

maybe we can find a packge name that could hold both additions

[burner]

@jmdavis what about the name "auditing" as a parent package name to collect this PR and any further testing/auditing modules.

std.(experimental.)auditing.stopwatch;
std.(experimental.)auditing.randomized_unittest_benchmark;
...

[jmdavis]

@burner I think that benchmark is better than auditing. Auditing tends to imply that financial stuff is involved. Also, the current benchmark function and StopWatch type aren't really verifying anything. They're just used for timing stuff. So, "timing" would be another option, but it's more vague, and folks could mistakenly think that it has something to do with more general time stuff.

[burner]

I know, but randomized_unittest_benchmark does more than just benchmark, it also tests functions. IMO we need a word that encompasses benchmarking and testing.

update:
and maybe even later an int wrapper with NaN value. basically everything that help to find bugs and bottlenecks in your program

[andralex]

Yes, this should go in experimental. We need to beef things up a lot before putting it in std. There was all about talk (and a failed PR) related to providing a true benchmarking facility that goes through an entire module and benchmarks it using primitives defined by user code.

[andralex]

I'm removing batsignal because I stated my say...

[burner]

@andralex I think I do not follow. Could you please elaborate.

[wilzbach]

I think what @andralex wanted to say is that once this API is "freed", he doesn't want it directly be put into baked stone yet.
@jmdavis - would you be so kind to put it to experimental, so we can get it merged? :)

[wilzbach]

ping @jmdavis

[wilzbach]

@jmdavis - would you be so kind to put it to experimental, so we can get it merged? :)

ping pong ;-)

[JackStouffer]

Closing due to almost nine months of inactivity.

[skl131313]

Should StopWatch and benchmark() be deprecated then? Seems they only aren't cause @jmdavis wanted a replacement to be in place for them first. But that doesn't seem to be happening anymore.