Skip to content
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

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

Closed
rogpeppe opened this issue Feb 8, 2017 · 15 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rogpeppe
Copy link
Contributor

rogpeppe commented Feb 8, 2017

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
Copy link
Contributor

bradfitz commented Feb 8, 2017

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?

@bradfitz bradfitz added this to the Go1.9 milestone Feb 8, 2017
@rogpeppe
Copy link
Contributor Author

rogpeppe commented Feb 8, 2017

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
Copy link
Contributor

rsc commented Feb 8, 2017

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
Copy link
Contributor Author

rogpeppe commented Feb 8, 2017

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
Copy link
Contributor

rsc commented Feb 8, 2017

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
Copy link
Contributor

rsc commented Feb 8, 2017

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.

@rsc rsc changed the title time: truncated times don't imply truncated time differences time: trim monotonic clock reading in Round, Truncate, In, Local, UTC? Feb 8, 2017
@cespare
Copy link
Contributor

cespare commented Feb 8, 2017

See #18996 for Duration.Truncate.

@rogpeppe
Copy link
Contributor Author

rogpeppe commented Feb 8, 2017

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
Copy link
Contributor

neild commented Mar 1, 2017

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
Copy link
Contributor

dmitshur commented Mar 1, 2017

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
Copy link
Contributor

rsc commented Mar 2, 2017

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

gopherbot pushed a commit that referenced this issue Mar 2, 2017
The original analysis of the Go corpus assumed that these
stripped monotonic time. During the design discussion we
decided to try not stripping monotonic time here, but existing
code works better if we do.

See the discussion on golang.org/issue/18991 for more details.

For #18991.

Change-Id: I04d355ffe56ca0317acdd2ca76cb3033c277f6d1
Reviewed-on: https://go-review.googlesource.com/37542
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@dmitshur
Copy link
Contributor

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 bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 23, 2017
@bradfitz
Copy link
Contributor

@rsc, decision?

@rsc
Copy link
Contributor

rsc commented Jun 5, 2017

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.

@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 5, 2017
@gopherbot
Copy link

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

@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants