Navigation Menu

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

Jump to bottom

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
Labels
FrozenDueToAge release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages)
Milestone
Go1.13

Comments

@dsnet
Copy link
Member

dsnet commented Aug 16, 2019 •
edited

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 release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages) labels Aug 16, 2019
@dsnet dsnet added this to the Go1.13 milestone Aug 16, 2019
@dsnet dsnet self-assigned this Aug 16, 2019
@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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
@dsnet
time: update TestSub to avoid future regressions
c485506 
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 pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@dsnet @t4n6a1ka
Revert "time: optimize Sub"
b02b1c9 
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 pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@dsnet @t4n6a1ka
time: update TestSub to avoid future regressions
d640a0b 
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 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
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages)
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
3 participants
@pongad @dsnet @gopherbot