-
Notifications
You must be signed in to change notification settings - Fork 18k
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
proposal: time: add Instant type exposing Monotonic Time #61765
Comments
The whole point of https://go.googlesource.com/proposal/+/master/design/12914-monotonic.md was to avoid exposing monotonic clocks. |
OK...AND... this is a new proposal to expose them, for good reasons. I do not agree that it should not be exposed. |
The linked earlier proposal that added monotonic time as an implementation detail describes the rationale for not exposing a separate monotonic API: that having two different APIs which behave exactly the same except during very occasional events like a leap second invites bugs where people use the wrong one. However, I can see some subtlety in that argument: having two separate APIs where one is a strict subset of the other would seem to avoid the bug hazard and instead offer two APIs that both correctly handle monotonic time but where one of them is free of the overhead of also tracking wallclock time. That seems like a plausible argument in theory, but I expect the best way to argue the value of such an addition would be to demonstrate:
I don't have a strong opinion about this myself. I offer the above just as an idea for how to add new information that might motivate revisiting the conclusions from that earlier proposal. It isn't typically sufficient only to disagree with the previous outcome without adding new information. |
I have read the paper before and what it's describing sounds very much like the different of I'll refer to Rust's types because I think their documentation explains it better than I could ever. I understand the rational of trying to limit the API surface area and hiding some lower level concepts and complexity under Maybe My proposal should be to add both In either case I am only interested in Monotonic time at this moment, here are some performance reasons, using the existing linked code in the proposal, I think this new type would benefit some real world libraries involving as caches, benchmarking and metrics off the top of my head. func BenchmarkTime(b *testing.B) {
for i := 0; i < b.N; i++ {
instant := time.Now()
_ = time.Since(instant)
}
}
func BenchmarkInstant(b *testing.B) {
for i := 0; i < b.N; i++ {
instant := timeext.NewInstant()
_ = instant.Elapsed()
}
}
--- Result
goos: darwin
goarch: arm64
pkg: test
BenchmarkTime-8 16162813 66.93 ns/op 0 B/op 0 allocs/op
BenchmarkInstant-8 34626579 34.55 ns/op 0 B/op 0 allocs/op Just dealing with Instant and not the additional time same approximately 50% of the time, on my machine at least. But that's not the whole story either, there's also the size different of representing each in memory: fmt.Println(unsafe.Sizeof(time.Now()))
fmt.Println(unsafe.Sizeof(timeext.NewInstant()))
-----
// 24
// 8
|
For prior art, @tailscale has |
From an API standpoint, For space, I find it very hard to believe the 24 vs 8 bytes matters at all. No one is making giant arrays of Instants. They're only subtracting them. People who really really care about the space can declare
and then store arrays of durations since globalStart. For cpu, there is a factor of two in querying the time, since time.Now fetches wall time and monotonic time, and both have roughly the same cost (the RDTSC or equivalent, which takes O(10-30ns)). Later, when Since is used, the code already knows to fetch only monotonic time when that's all it needs. So for a Now+Since pair, we are fundamentally talking about optimizing 3 time fetches down to 2, or making the sequence 33% faster. (Benchmark at end confirming my claims.) So how can we get rid of that extra fetch? People who really need to optimize away 50% of time.Now today already can do that:
(tstime/mono.now could do the same thing – Given that there is a portable way to optimize away the one extra RDTSC already, I don't think it's terribly urgent to make an API change here, especially one that doubles the number of time types. In the long term, I wonder if we can pressure the operating systems to add a function that gets a matched pair of both times. I'm not sure how much Windows or macOS matter anyway. For Linux the kernel vdso provides the accessors, and the two accessors do exactly the same thing except for which offset they add. Perhaps we could convince the Linux kernel team to accept a patch adding a "get both" call. Benchmark
|
This proposal has been added to the active column of the proposals project |
CL 518135 has an optimization of time.now on Linux that avoids the second RDTSC in exchange for a few extra seqlock checks: instead of doing two regular clock_gettime, it does one regular clock_gettime and three coarse clock_gettime.
If (1) and (4) match, then 1, 2, 3, and 4 are all based on the same vdso time parameter update, meaning we can derive accurate monotime = (3) - (1) + (2). On a relatively old (2014) Intel chip:
On a relatively new (2022) AMD chip:
That is, on the old Intel chip, with the optimization, Now+Since takes (31+22)/(21+21) = 1.25X longer than optimal, It is still difficult for me to understand the situation where 15ns overhead (for just time.Now) is no big deal but 30ns is a problem. Or where 30ns (for Now+Since) is no big deal but 45ns is a problem. If we can drop the comparisons to 15 vs 19 and 31 vs 36 then I assume that situation gets even less compelling as a rationale Anyone is welcome to check out CL 518135 and see how it speeds up their systems. It is linux/amd64-only. |
And again anyone who really needs direct monotonic time access at the fastest possible speed already has a portable way to do that (time.Since against a reference time). |
Curious, how would/does the existing |
Very cool, didn't know that when So I'm convinced that the existing API can be leveraged to get the same speedup as direct monotonic time access given all the above :)
Wondering if the conversation could be shifted then from performance to easy of use. The following 100% works but is a little verbose to write, maybe that's ok in the pursuit of this kind of performance, but wanted to float the idea of leveraging existing What would you think about something like this? var base = time.Now()
// Instant represents a monotonic instant in time.
//
// Instants are opaque types that can only be compared with one another and allows measuring of duration.
type Instant int64
// NewInstant returns a new Instant.
func NewInstant() Instant {
return Instant(time.Since(base))
}
// Elapsed returns the duration since the instant was created.
func (i Instant) Elapsed() time.Duration {
return time.Since(base) - time.Duration(i)
}
// Since returns the duration elapsed from another Instant, or zero is that Instant is later than this one.
func (i Instant) Since(instant Instant) time.Duration {
if instant > i {
return 0
}
return time.Duration(i - instant)
} Then could be use like so just as before just some complexity hidden under the hood: start := time.NewInstant()
...
d := start.Elapsed() An example of code clarity using var maxCacheAge time.Duration
type cacheNode struct {
...
added time.Instant
}
vs
type cacheNode struct {
...
added time.Duration
} Although the Again floating the idea and would be curious your thoughts. |
time.Time uses monotonic times when available (all the major operating systems) and otherwise falls back to non-monotonic time.
I'm convinced we can get the same speedup as direct monotonic time access by optimizing time.Now. It may take a little while to get completely down to the same speed, but new APIs last forever. I don't think we need to add new API.
I think that kind of Instant type is fine to define in your own package. Once we fix the speed of time.Now, the only benefit of Instant will be that it's 8 bytes instead of 24. If you have a ton of these, maybe that matters. In that case, defining your own smaller type makes sense. That is, given the choice between time.Instant and time.Duration in the cacheNode, I would choose time.Time instead. If a time.Time was too big, then I would define my own unexported type, private to the cache, justified by whatever optimizations I wanted. Maybe it would even make sense to coarsen it to 4 bytes, depending on context. That's a good decision to make inside a specific package. I agree that direct use of Duration is not the clearest way to write that code. If we add Instant to package time, it is absolutely a new API. I don't think "storing so many times that 24 bytes is too much" comes up often enough in practice to be worth new API in package time. In your own API, anything you want is fine. 😄 That said, for my reading I don't think Since is the right name. In package time, Since is a top-level function: time.Since(t) is clear and means what the English phrase "time since t" means. When Since is a method on Instant, then i.Since(j) is not clear at all and has no English analogue. It seems like i.Sub(j) would be clearer (and align with Durations). |
Change https://go.dev/cl/518336 mentions this issue: |
time.Since(base) is an idiom that can be used to read the system monotonic time as efficiently as possible, when that matters. The current code structure adds a few nanoseconds on top of the 15-20ns the time read already takes. Remove those few. After this CL, there is no reason at all for anyone to //go:linkname runtime.nanotime1 instead. Came up while investigating #61765. Change-Id: Ic9e688af039babfc2a5a8e67dcbb02847a5eb686 Reviewed-on: https://go-review.googlesource.com/c/go/+/518336 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Damien Neil <dneil@google.com> Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
time.Since(base) is an idiom that can be used to read the system monotonic time as efficiently as possible, when that matters. The current code structure adds a few nanoseconds on top of the 15-20ns the time read already takes. Remove those few. After this CL, there is no reason at all for anyone to //go:linkname runtime.nanotime1 instead. Came up while investigating golang#61765. Change-Id: Ic9e688af039babfc2a5a8e67dcbb02847a5eb686 Reviewed-on: https://go-review.googlesource.com/c/go/+/518336 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Damien Neil <dneil@google.com> Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Joseph Tsai <joetsai@digital-static.net> (cherry picked from Go commit 4467839) See tailscale/tailscale#8848
Based on the discussion above, this proposal seems like a likely decline. |
Change https://go.dev/cl/520065 mentions this issue: |
Change https://go.dev/cl/521237 mentions this issue: |
This is similar to CL 518336. For #61765. Change-Id: I7c1d92a3b3e2b6c1c0058a2094997d93082ad139 Reviewed-on: https://go-review.googlesource.com/c/go/+/521237 Run-TryBot: Ian Lance Taylor <iant@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
No change in consensus, so declined. |
This will be short a suite as it's pretty basic.
There is appetite to have access to Monotonic Time in Go from previous issues, proposals and requests.
Proposed Additions
I propose adding a new type to the
time
package calledInstant
which will represent an opaque type that can only be compared with one another and allows measuring of duration. An existing implementation can be found here exert posted below for convenience.This will allow access to Monotonic Time from Go, that already exists, but yet unexposed except through
time.Time
. The reason this is necessary is thattime.Time
is a very heavy structure and has a bunch of overhead that is not necessary if only measuring monotonically increasing time such as when used in Benchmarking and very hot code paths.The text was updated successfully, but these errors were encountered: