time: trim monotonic clock reading in Round, Truncate, In, Local, UTC?

[rogpeppe]

An interesting ramification of the new monotonic time issue, perhaps not solvable.

I just wrote some code that looked a bit like this:

 t0 := time.Now().Truncate(time.Millisecond)
 something()
 t1 := time.Now().Truncate(time.Millisecond)
 fmt.Println(t1.Sub(t0))

and expected it to print a duration that's a whole number of milliseconds but it doesn't, because of the variable skew between the monotonic and wall clock times.

https://play.golang.org/p/pn2AWxrRU0

go version devel +5f374ea Mon Feb 6 22:45:49 2017 +0000 linux/amd64

[bradfitz]

Maybe Truncate should also remove the monotonic bit in its returned time? Or keep it but truncate its monotonic value? (which would be a unique case, though)

@rogpeppe, was that hypothetical or did it appear in real code?

[rogpeppe]

Well, it was real code that I just wrote and expected to work, not code that I wrote to demonstrate a problem I thought might exist.

If I did it, I bet others have done it too.

[rsc]

I was on the fence about whether this was a good idea. I did it this way to see what would break. Please keep reporting instances of this kind of thing. Probably we'll want to make Truncate and Round clear the monotonic reading for Go 1.9, but let's keep gathering data.

I wonder if @rogpeppe's example means that Duration should also have Round and Truncate. Then you could use time.Since(t0).Truncate(time.Millisecond), although I typically use fmt.Printf("%.3fs", time.Since(t0).Seconds()), which is nicer anyway.

[rogpeppe]

I would've used Duration.Truncate it it had existed.

The not-so-nice thing about fmt.Printf("%.3fs") is that it doesn't print so nicely for minutes and hours.

[rsc]

The printing is a great argument for Duration.Truncate. We should do that regardless. If someone wants to file an issue and/or send a CL, that's great.

[rsc]

I looked briefly and non-scientifically at calls to Now().Round/Truncate in go-corpus, and it does look like many of them would be broken by preserving the monotonic clock as we do today. I didn't check them originally because the original proposal was for them to strip the reading, along with In, Local, and UTC. @rogpeppe, you were the one who talked me into not stripping for In, Local, and UTC, and I added Round, Truncate to the list too. I should check on In, Local, UTC too. The argument for stripping in those cases is that you're in some sense explicitly asking to work in wall times.

[cespare]

See #18996 for Duration.Truncate.

[rogpeppe]

In, Local and UTC change the wall clock time independent of the monononic time, so I think they're OK. Truncate and Round make an adjustment that is dependent on the absolute value of the time, which is an argument for making them strip monotonic time, I think.

[neild]

Another case of code broken by Truncate preserving the monotonic clock:
https://github.com/youtube/vitess/blob/master/go/vt/throttler/interval_history.go#L41

The full context is spread around a few other files, but the short description is that this code is mapping times into windows and assumes that all times truncated to a given window are equal.

[dmitshur]

In case it's helpful, see google/go-github#568 for a real test case that fails on Go master in google/go-github repository. The change in that PR makes it pass.

Note that the original code uses != on time.Time values, rather than time.Equal, for doing comparisons. From package time documentation, using == operator on time.Time values seems to be discouraged, but not deemed illegal (from my reading of docs in 1.8 and latest master).

[rsc]

I'll send a CL for Round, Truncate; we can leave this issue open as a reminder to decide about In, Local, UTC.

[dmitshur]

I wanted to revisit one phrase in the time documentation after 6c85fb0.

If time values returned from time.Now and time values constructed by other means (for example, by time.Parse or time.Unix) are meant to compare equal when used as map keys, the times returned by time.Now must have the monotonic clock reading stripped, by setting t = t.AddDate(0, 0, 0).

After 6c85fb0, there are 3 ways to strip monotonic clock reading:

• by setting t = t.AddDate(0, 0, 0).
• by setting t = t.Round(0).
• by setting t = t.Truncate(0).

I wanted to ask if suggesting t = t.AddDate(0, 0, 0) as a way of stripping monotonic clock reading is still the best option out of the three, after 6c85fb0.

I think it's fine, but just wanted to ask.

[bradfitz]

@rsc, decision?

[rsc]

In Google's code base, fixing UTC and Local to strip the monotonic reading would fix about half as many tests as fixing Truncate / Round did (but different ones). It seems worth doing. I will send a CL.

[gopherbot]

CL https://golang.org/cl/44858 mentions this issue.