Add std.time.nanoTimestamp function
#5237
Conversation
|
You should be able to use `Once` (#5094)
…On Fri, 1 May 2020 at 12:47, LeRoyce Pearson ***@***.***> wrote:
Implementing this for #4635 <#4635>
------------------------------
You can view, comment on, or merge this pull request online at:
#5237
Commit Summary
- Add `std.time.nanoTimestamp` function
File Changes
- *M* lib/std/time.zig
<https://github.com/ziglang/zig/pull/5237/files#diff-131d67764891bff8850d6083359b0151>
(32)
Patch Links:
- https://github.com/ziglang/zig/pull/5237.patch
- https://github.com/ziglang/zig/pull/5237.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#5237>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA7HYYXFLQEFX6SKTP3TH3RPIZ4TANCNFSM4MWZ5IAQ>
.
|
Ah, okay. Thanks! |
There was a problem hiding this comment.
Thanks for the patch - this is turning into quite a scavenger hunt! 3 things here:
- This stack overflow answer regarding Darwin is interesting. I haven't taken the time to fully understand and audit it yet. Maybe that is the best way to implement
nanoTimeStamp().
However, in terms of the cache hash project, I think this is not what we want. What we need is to call the same OS get time function that the file system will be using, so that we can check if the mtime will have a problematic timestamp. It's not heuristic-based - it's truly correct - as long as it gets the same time source as the file system. And I'm pretty sure the darwin filesystem is usingclock_get_time(the most-upvoted stack overflow answer on that question) rather than doing this additional bookkeeping here. That's also what os.cpp in stage1 does, which empirically is working correctly.
My suggestion to move forward on point (1) is to treat this std/time.zig patch as a separate contribution, unrelated to the cache hash feature you are working on, (which I will be happy to continue helping with the review & merge process if you are interested), and take a different strategy for unblocking cache hash stuff.
The detectProblematicTimestamp function is tightly coupled to the file system, and needs to use the same "get time" OS function as the file system. Because of this I think that std.time may actually be too high level (as evidenced by the fact that it might make sense to take this darwin implementation code), and the cache hash code should resort to using the posix layer (std.os) with perhaps some windows-specific code using GetSystemTimeAsFileTime. In os.zig, clock_gettime should probably use clock_get_time on Darwin, and if it makes sense, it could even use GetSystemTimeAsFileTime on Windows. In this case it would make sense for detectProblematicTimestamp to reach directly into std.os.clock_gettime.
Does that make sense?
-
Is
i64big enough to store nanoseconds? C APIs use a struct with 2 fields, amounting to 16 bytes on 64 bit architectures. Should we perhaps be using i128 (or a struct with two fields like C)? The maximum valuei64in nanoseconds, is only 292 years, which seems like a problem. And not like a y2k problem - I think it's already possible to modify the timestamp of a file to be 300 years into the future right? So this value seems like it is not big enough. Maybe ani96would be enough; that comes out to 11255295079474 years. -
Rather than underscore prefixes, please use words that explain the situation. Maybe
global_timestartrather than_timestart, and the_init_timestart_onceis self-explanatory and can have the underscore prefix dropped.
| const sec_ms = tv.tv_sec * ms_per_s; | ||
| const usec_ms = @divFloor(tv.tv_usec, us_per_s / ms_per_s); | ||
| return @intCast(u64, sec_ms + usec_ms); | ||
| // https://stackoverflow.com/a/21352348 |
There was a problem hiding this comment.
I'm confused why we need this at all.
OSX 10.13+ has a working CLOCK_REALTIME.
This code is only required to support <= OSX 10.12 (which is unsupported by zig in general).
I guess we can keep it for older OSX, in which case we should put it behind a version check:
if (switch(std.Target.current.os.tag) {
.macosx => std.Target.current.os.version_range.semver.min.major == 10 and std.Target.current.os.version_range.semver.min.minor < 12,
// TODO: figure out which version CLOCK_REALTIME was added,
.ios, .watchos, .tvos => false,
else => false,
}) {
lib/std/time.zig
Outdated
| var _timestart: DarwinTimeStart = undefined; | ||
| var _init_timestart_once = std.once(_init_timestart); | ||
|
|
||
| pub fn _init_timestart() void { |
There was a problem hiding this comment.
probably doesnt need to be pub? also remove the _.
lib/std/time.zig
Outdated
| initclock: u64, | ||
| }; | ||
|
|
||
| var _timestart: DarwinTimeStart = undefined; |
There was a problem hiding this comment.
no need for the _ prefix.
| var global_timestart: DarwinTimeStart = undefined; | ||
| var init_global_timestart_once = std.once(init_global_timestart); | ||
|
|
||
| pub fn init_global_timestart() void { |
There was a problem hiding this comment.
I think this shouldn't be pub? If we want to permit people to pre-load it then make init_global_timestart_once pub.
|
|
||
| /// Get the posix timestamp, UTC, in nanoseconds | ||
| /// | ||
| /// On windows this only has a granularity of 100 nanoseconds. |
There was a problem hiding this comment.
FYI posix has clock_getres to indicate the granularity of the timer. We should probably wrap that too and mention it in this comment.
daurnimator
added
the
standard library
Implementing this for #4635