Add ClockType enum to core.time for issue# 13433. #990
Conversation
| alias CLOCK_MONOTONIC_FAST coarseClock; | ||
| } | ||
| else | ||
| assert(0, "What is the COARSE monotonic clock for clock_gettime for this system?"); |
There was a problem hiding this comment.
I wonder if this jives with the description that:
If the function in question does not support the given clock (or it does on some systems but not the current system), then ClockType.normal is used.
I understand what you are trying to do, but I wonder if it should just be in a unit test instead of in the main code, so the developers are aware of it, but it doesn't impede adoption on a new platform.
There was a problem hiding this comment.
@schveiguy I think that the description and implementation jive just fine. The implementation is taking the normal approach of erroring out on new systems so that the developers there are forced to make the correct decision for that platform (either add the coarse clock if there is one or provide a branch of the implementation for that platform which doesn't use the coarse clock). From the user's perspective, the coarse clock may or may not work on the new system, but the developer is forced to deal with it.
The alternative is to just not provide a coarse clock on new platforms unless someone realizes on their own that it's an issue and implements it for that platform, and that's not normally how we deal with new platforms. In this case, it would likely mean that no new platforms would support the coarse clock even if they can and should.
As for just having unit tests, I don't see how that really tells the developers anything. At best, that means that if they're glancing through core.time and happen to see those tests and happen to wonder whether they need to do something for their specific platform and then look at the implementation and see that they do, I don't see how they're going to catch it. An explanatory comment would help with that, but they'd still have to be looking. It's common practice in druntime and Phobos to use static assertions for new platforms so that developers will know that they need to do something for that platform when it's added rather than assuming that everything is fine.
Now, I should probably rewrite this code a little so that the clock decision for clock_gettime is made in the version branches rather than in the call to clock_gettime so that it's easier to add a new POSIX platform with no coarse clock and so that other clocks can be added to specific platforms more easily later if we decide to do that, but I think that the basic approach here is fine and that it's in line with how we normally do things. The fact that the description says that the coarse clock is not necessarily supported doesn't really matter insofar as the developers have to figure out whether it's appropriate for the new system regardless, and without the assertion, they won't catch that.
There was a problem hiding this comment.
Now, I should probably rewrite this code a little so that the clock decision for clock_gettime is made in the version branches rather than in the call to clock_gettime
Yes, this will look better. Much better than my idea of using a unittest.
Just get rid of else version(posix) and the aliases to the coarseClock. It looks a bit messy currently, and this will make the code more consistent and readable. And as you said, clock options on other platforms will not affect Linux or FreeBSD.
What I was thinking about is, on a posix but not linux or freebsd platform, the default works, but the coarse clock just fails, and at runtime no less. This seems inconsistent to me. A much better situation would simply be to make the developer of the new platform figure it out during porting stage.
There was a problem hiding this comment.
Ah, I see what you mean now. I intended for it to be a static assertion, but it's a regular assertion. It's supposed to fail at compile time. That's what we typically do with the else branch of a version block in druntime and Phobos. I just screwed up and forgot the static.
|
Okay. I updated it so that the clock is chosen in the version branches, and I fixed it so that the assertion is static like it's supposed to be. |
| auto clockArg = type == ClockType.normal ? CLOCK_MONOTONIC : CLOCK_MONOTONIC_FAST; | ||
| } | ||
| else | ||
| static assert(0, "What is the COARSE monotonic clock for clock_gettime for this system?"); |
There was a problem hiding this comment.
OK, 2 new comments :)
- I think the clockArg should be by default normal, not by default coarse. We don't know what "other" clocks would represent on other platforms. The way the code is now, it by default selects coarse. I'm wondering actually if it shouldn't be a final switch, so any new clocks added force the developer to handle those cases specifically.
- The static error message seems to focus only on COARSE, but it's possible all the clock defines may be differently named. I would change it to "Please handle all the clock types for this system."
|
Okay. I fixed it up again. Now it won't accidentally do coarse if a new clock type is added. I didn't use a final switch, because that would be a fair bit of extra code, and I don't think that it adds much value here. New platforms need the error from the static assertion so that the developers who are adding them know that they need to add the code for that platform, but anyone adding a new value to |
| assert(abs(coarse2 - norm1) <= seconds(1)); | ||
| assert(abs(coarse1 - norm2) <= seconds(1)); | ||
| assert(abs(coarse1 - norm1) <= seconds(1)); | ||
| assert(abs(norm2 - norm1) <= seconds(1)); |
There was a problem hiding this comment.
OK, I'm ready to say this is good to go, but I'm curious. I see some potential, very slight but still there, that these tests may fail, based on the usage of the system being tested on (which would be a false failure). Is there a good reason to have these tests?
There was a problem hiding this comment.
What types of potential failures do you foresee? It's basically making sure that the values coming from the clocks are essentially the same. It would be very bad for instance if the coarse clock returned a completely different value than the the normal clock, because then when you compare the MonoTimes, you'd be comparing completely unrelated values. The coarse clock should just be updating less frequently and if the a coarse time and a normal time are close enough together, then it may look like the coarse one went backwards, but we don't want something like the coarse clock having 1000 and the normal one having 5000000 for approximately the same time - though maybe the solution to that is to add the clock type to MonoTime internally and throw an Error if MonoTimes with different clock types are compared... Then it would be possible to use totally different clocks for different MonoTimes without it being caught. It's possibilities for weirdness with that that made it so that I didn't add any ClockTypes for stuff like Linux's CLOCK_MONOTONIC_RAW or CLOCK_BOOTTIME, but if MonoTime has the clock type, that problem goes away. I'll have to consider that then.
Regardless, it looks like I missed the assert(coarse1 <= coarse2);, so I'll need to fix that.
There was a problem hiding this comment.
What types of potential failures do you foresee?
We are depending on a certain execution speed. It's far-fetched, but imagine that the process is put to sleep in between reading the two clocks, and some other process hangs the OS for over 1 second. Then when it comes back, the delta between the two reads is more than 1 second, you get a failure.
It would be very bad for instance if the coarse clock returned a completely different value than the the normal clock
I don't know if we need to guarantee this possibility. Although it may be the case today, we are not in control of the meaning of these two clocks, and this can possibly change (very unlikely), or on a new system (non- linux or freebsd), there are two possible clocks, one "quick", and one "slow" to retrieve, but they are not related to each other.
I think the only thing we need to guarantee is that fetching two times from the same clock source is sane. I'd almost recommend putting a sleep in between those calls, and dropping the = part of <=.
though maybe the solution to that is to add the clock type to MonoTime internally and throw an Error if MonoTimes with different clock types are compared...
I don't like that, this introduces another possible runtime check, and possibly another member to the struct.
Overall, I'm not vehemently opposed to the tests, I just was pointing out possible issues with them. In any case, unit testing time features is going to largely depend on the behavior of the OS, over which we have 0 control.
There was a problem hiding this comment.
We are depending on a certain execution speed. It's far-fetched, but imagine that the process is put to sleep in between reading the two clocks, and some other process hangs the OS for over 1 second. Then when it comes back, the delta between the two reads is more than 1 second, you get a failure.
Ah, true. That could definitely happen, but I'm loathe to use sleeps in general unless I have to, and I'm trying to specifically test that the clocks are very close to one another (just differing in precision, not units), so sleeps would actually defeat the purpose of the tests. So, I suspect that we're stuck the possibility of that random failure, though I think that it's unlikely - especially on the autotester where the boxes are dedicated to running the tests.
I really don't like the idea of MonoTime being able to be backed by two completely different clocks without any way to differentiate the resulting MonoTimes, because that could easily lead to weird behavior and nasty bugs if the clocks are mixed. I'm okay with using the coarse clock so long as it appears to just be grabbing the time less frequently at the kernel level and thus not be as up-to-date (which seems to be what's happening on both Linux and FreeBSD), but if they're completely different timers, then that's just begging for bugs IMHO to use MonoTime for both without a member variable differentiating them. So, that means either creating separate types for the different clocks or adding a member variable to differentiate them. Neither choice is particularly appealing, though between the two of them, I'd much prefer the member variable route. So, to avoid that, we need to ensure that the clocks being used have the same start time and units (or very close to it anyway, since we can't actually test for that directly). Without that, I think that we need to either add a member variable indicating the clock type or to not support the coarse clock (or any clock which does not have the same start time and units). Fortunately, it looks like the coarse clock is really just the same clock as the normal one except that it's grabbing the time from a place that isn't updated as frequently, so I think that we can support the coarse clock without having to worry about mixing clocks, but I think that it's then unlikely that any of the other clock types from the specific implementations of clock_gettime will be supported (except maybe CLOCK_MONOTIME_PRECISE on FreeBSD, but I haven't been able to find any info on what's different between that and the normal clock, which is supposed to be precise already).
I should probably better comment the tests though to make the intention clear.
There was a problem hiding this comment.
because that could easily lead to weird behavior and nasty bugs if the clocks are mixed.
Well, from my understanding the "coarse" time is derived from the same clock as the monotonic time, but just captured once every tick by the OS instead of on demand. This means it's possible to read the monotonic time, and then read the coarse time, and get a negative delta. I think at the very least, we should warn people that we do not guarantee different clocks are exactly in sync.
Just a recommendation to only use one clock consistently would be good.
There was a problem hiding this comment.
Yeah. That matches my understanding, and I agree that clocks shouldn't be mixed, but mixing the coarse and normal clocks should mostly work (particularly if you're dealing with values grabbed at least a millisecond or two apart), but other clocks would be a different story entirely, since their values wouldn't even necessarily mean the same thing. I'll add a note about not mixing clocks though.
|
Okay. Updated. |
|
Looks good to merge. Although in this state MonoTime locks out any systems which can provide a coarse clock through other means than just delaying the call to the precise clock. (MonoTime requires same base, same frequency and same wrap-around behavior, correct?) I'm just wondering because I don't know if |
|
|
|
Okay. Documentation updated to note that it should be considered an error if |
|
Another option would be to use a separate type per clock domain, e.g. by templating MonoTime on ClockType. |
Hmm. Part of me thinks that that's a great idea, and part of me thinks that that's a horrible idea. It would definitely clean things up from the perspective of how handling potentially drastically different monotonic clocks would work, but it also introduces an extra level of complication in that I'll have to think about it, but it's a tempting solution.
|
|
You can make You can always compare two unrelated times by comparing the hnsecs. I'm for @MartinNowak's idea, if it helps. |
I was wondering whether adding more monotonic clock sources was overengineering, but apparently it's much faster to obtain a coarse clock than a finer one. |
Weird that you cannot use a templated |
I would still argue against that -- MonoTime is really named after the clock source -- monotonic clock. If we go with a templated solution, and you can't compare two different template instance types, then it would be nice to be able to use the same API for all clocks on the system that are available. |
It is faster, but it's precision is down to about a millisecond, which means that it's really only useful when you want to ask for the time very frequently but don't care about it being precise, which seems like a pretty weird use case to me.
Ouch. If I new that, I'd forgotten about it. That sucks. LOL. We're basically forced to do what C++ does with std::string.
Linux has something like 4 or 5 different clocks which are all monotonic, and |
|
Well, I like to keep options open :) I think that renaming it later probably will not be super-painful, but if we can find a name now that is generic enough not to be renamed later, that would be better. Most users will never even view this type, and avoid it completely, just using the vanilla MonoTime. As you said, the use cases are not plentiful, but there are very good ones. The fact that CLOCK_MONOTONIC_COARSE didn't even exist until a few years ago is a testament to that. |
Profiling with low overhead for example or request timings in a web app. |
|
Okay. I just did a major rewrite. With Maybe all of these clock types is taking it a bit far, but it is thorough enough that folks on Linux and FreeBSD shouldn't need to go to |
|
|
||
| $(LREF MonoTimeImpl) and $(D Clock.currTime) take the $(D ClockType) as a | ||
| template argument. In the case of $(LREF MonoTimeImpl), the alias | ||
| $(LREF MonoTime) (which is $(D MonoTimeImpl!(Clock.currTime))) is provided |
There was a problem hiding this comment.
MonoTimeImpl!(ClockType.normal)
|
Okay. I merged in Steven's fix and rebased so that we wouldn't have a slew of commits for this. Hopefully, it finally passes the autotester this time. |
See #1238 |
|
Nuts. It fails on FreeBSD. I can't see why it would... If it's failing on Linux too, I'll look at see if there's something I messed up. |
I thought that I'd tested it locally. I guess not. And I might have screwed something up when I merged it, since I did make a few minor tweaks (e.g. change some of the |
| if(_ticksPerSecond == 0) | ||
| assert(0, "MonoTime failed to get the frequency of the system's monotonic clock."); | ||
| if(ticksPerSecond == 0) | ||
| assert(0, "MonoTimMonoTimeImpl failed to get the frequency of the system's monotonic clock."); |
There was a problem hiding this comment.
Hm... MonoTimMonoTimeImpl? :)
There was a problem hiding this comment.
You could probably use that nifty new _clockIdx to get the name of the clock for this message :)
|
Hah, It could be another wonderful FreeBSD bug :) I'll deprecate the test just in case. |
It looks like an invalid |
No, that's not it. I mixed up |
|
No. I think that the problem is |
|
No need to do that I think, just change to this: mixin("auto clockArg = ClockType." ~ typeStr ~ ";"); |
|
I'm surprised if the original doesn't work though, I thought alias would be OK within each scope, but maybe it has weird interactions with static foreach. |
No. That's wrong. The value of |
|
Okay. That's been fixed, and I fancied up some of the assert messages to use the name of the |
Huh? That's the argument you are passing to clock_getres. Since I'm not instantiating the template any more, there's no need to have the actual alias. Edit: totally understand now. Sorry. |
Yes. It's the value that's being passed to |
It happens. |
| mixin("static if(clockType == ClockType." ~ val ~ `) return "enum name = \"` ~ val ~ `\";";`); | ||
| assert(0); | ||
| } | ||
| mixin(genMix()); |
There was a problem hiding this comment.
You can actually now replace all this with:
enum name = __traits(allMembers, ClockType)[_clockIdx];This adds an enum for indicating which type of clock to use when it's appropriate for a time function to have multiple options for the source clock. In the case of MonoTime, to make that work cleanly, the implementation of MonoTime has become MonoTimeImpl, templated on ClockType, and MonoTime has become an alias to MonoTimeImpl!(ClockType.normal). In the case of SysTime (in a separate PR), that will a default template argument to Clock.currTime and SysTime will be unaffected (because in MonoTime's case, the clock that it came from is integral to the type, whereas in SysTime's case, it doesn't matter after the SysTime has been initialized).
|
Hot damn! it passes :) I'm good with merging this, I'll let others review since it's changed a bunch today. |
|
Auto-merge toggled on |
Add ClockType enum to core.time for issue# 13433.
| else static if(clockType == ClockType.processCPUTime) alias clockArg = CLOCK_PROCESS_CPUTIME_ID; | ||
| else static if(clockType == ClockType.raw) alias clockArg = CLOCK_MONOTONIC_RAW; | ||
| else static if(clockType == ClockType.threadCPUTime) alias clockArg = CLOCK_THREAD_CPUTIME_ID; | ||
| else static assert(0, "ClockType." ~ typeStr ~ " is not supported by MonoTimeImpl on this system."); |
There was a problem hiding this comment.
This breaks the documentation build because with version (CoreDdoc) the enum contains all members.
I'd suggest to initialize the clock resolutions lazily instead.
|
Interesting benchmark numbers to get the time with different clocks. |
This adds an enum for indicating which type of clock to use when it's
appropriate for a time function to have multiple options for the source
clock. Currently, it only supports "normal" and "coarse," but we may add
more in the future, since some systems do support others (e.g
clock_gettime has several other options depending on the system).
This is in support of dlang/phobos#2584.