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: Time.Sub regression #33677

Closed
dsnet opened this issue Aug 16, 2019 · 4 comments
Closed

time: Time.Sub regression #33677

dsnet opened this issue Aug 16, 2019 · 4 comments
Assignees
Milestone

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Aug 16, 2019

Consider the following:

func main() {
	t := time.Date(2311, 11, 26, 02, 16, 47, 63535996, time.UTC)
	u := time.Date(2019, 8, 16, 02, 29, 30, 268436582, time.UTC)

	fmt.Println(int(time.Second) + t.Nanosecond() - u.Nanosecond())
	fmt.Println(t.Sub(u))
}

On go1.12, this prints:

795099414
2562047h47m16.795099414s

On go1.13beta1, this prints:

795099414
2562047h47m16.854775807s

Notice how the nanoseconds are off? On 1.12, it is correctly .795099414, while on 1.13, it is incorrectly .854775807. This regression causes the calculation of timeouts for network connections to go wonky in RPC implementations.

The culprit for this regression is https://golang.org/cl/131196

\cc @pongad @ianlancetaylor @cybrcodr @paranoiacblack

@dsnet dsnet added this to the Go1.13 milestone Aug 16, 2019
@dsnet dsnet self-assigned this Aug 16, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 16, 2019

Change https://golang.org/cl/190497 mentions this issue: Revert "time: optimize Sub"

@gopherbot gopherbot closed this in 4983a0b Aug 16, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 16, 2019

Change https://golang.org/cl/190524 mentions this issue: time: add TestSub test case to avoid future regressions

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 16, 2019

Change https://golang.org/cl/190524 mentions this issue: time: update TestSub to avoid future regressions

gopherbot pushed a commit that referenced this issue Aug 16, 2019
CL 131196 optimized Time.Sub, but was reverted because
it incorrectly computed the nanoseconds in some edge cases.
This CL adds a test case to enforce the correct behavior
so that a future optimization does not break this again.

Updates #17858
Updates #33677

Change-Id: I596d8302ca6bf721cf7ca11cc6f939639fcbdd43
Reviewed-on: https://go-review.googlesource.com/c/go/+/190524
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
This reverts commit CL 131196 because there is a bug
in the calculation of nanoseconds.

Fixes golang#33677

Change-Id: Ic8e94c547ee29b8aeda1b9a5cb9764dbf47b14b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/190497
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
CL 131196 optimized Time.Sub, but was reverted because
it incorrectly computed the nanoseconds in some edge cases.
This CL adds a test case to enforce the correct behavior
so that a future optimization does not break this again.

Updates golang#17858
Updates golang#33677

Change-Id: I596d8302ca6bf721cf7ca11cc6f939639fcbdd43
Reviewed-on: https://go-review.googlesource.com/c/go/+/190524
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@pongad
Copy link
Contributor

@pongad pongad commented Nov 25, 2019

Wanted to give a little update on this. (Apologies if this should be written elsewhere.)

There is a fix for this that passes all tests by checking t.Before(u) against the sign of the computed duration. At least on my laptop, the performance benefit of this is rather small at ~20% in an already-fast function. I'm not sure if this is actually worth doing. Unless someone can think of a better algorithm for this?

@golang golang locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.