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: print more than just "0" for a zero duration (revert?) #14058

Closed
robpike opened this issue Jan 21, 2016 · 19 comments
Closed

time: print more than just "0" for a zero duration (revert?) #14058

robpike opened this issue Jan 21, 2016 · 19 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@robpike
Copy link
Contributor

robpike commented Jan 21, 2016

http://play.golang.org/p/pIagpaYGak

package main

import "fmt"
import "time"

func main() {
    var t time.Duration
    t = 1e3
    fmt.Printf("%s\n", t)
    t -= 1e3
    fmt.Printf("%s\n", t)
    t -= 1e3
    fmt.Printf("%s\n", t)
    t = 1e6
    fmt.Printf("%s\n", t)
    t -= 1e6
    fmt.Printf("%s\n", t)
    t -= 1e6
    fmt.Printf("%s\n", t)
    t = 1e9
    fmt.Printf("%s\n", t)
    t -= 1e9
    fmt.Printf("%s\n", t)
    t -= 1e9
    fmt.Printf("%s\n", t)
}

prints

1µs
0
-1µs
1ms
0
-1ms
1s
0
-1s

It should show either ns because that's the internal resolution or s because that's the SI unit.

@robpike robpike self-assigned this Jan 21, 2016
@robpike robpike added this to the Go1.7 milestone Jan 21, 2016
@distributed
Copy link

(Somehow Google Groups ate my message, so here I go again)

+1. Personally I favor s for two reasons: 1.) As an engineer if in doubt I lean towards SI units and 2.) As a user of software written in Go I don't know about the internal resolution; I feel that s as a unit will be recognized as a duration with higher likelihood.

@tarndt
Copy link

tarndt commented Jan 24, 2016

It should show either ns because that's the internal resolution or s because that's the SI unit.

My vote is ns because it relays useful information- the precision of the quantity at hand.

Also, the adjacent values on the number line are labeled as ns, I would argue that fact favors zero being labeled similarly: http://play.golang.org/p/nWZN7w-2G9

@kkirsche
Copy link
Contributor

I would vote ns because that's the internal resolution. To me that makes sense as it's the lowest denominator and if a person is working in ns, µs or ms it seems odd to grow in magnitudes of the duration indicator when really it's 0 of the lowest possible value since the closest value would be 1ns not 1s

@mdempsky
Copy link
Member

In general, time.Duration.String does not use its internal resolution to indicate significant figures. For example, it formats time.Second as "1s", not as "1.000000000s" or "1000000000ns". Expecting the zero time.Duration's formatting to reflect its precision seems arbitrary and inconsistent.

Also, the godocs are explicit that the reason time.Nanosecond is formatted as "1ns" is to avoid the leading zero in "0.000000001s". There will be leading zeros in either "0s" and "0ns", so that does not seem like a valid argument to favor "0ns" either.

Lastly, time.Minute + 1 is formatted as "1m0.000000001s", not as "1m1ns", so it seems in general we favor formatting time.Duration values in units of seconds, not nanoseconds.

@rsc
Copy link
Contributor

rsc commented Apr 5, 2016

I suspect this should be 0s. Any SI prefix is distracting. There's no guarantee that it really is 0 to nanosecond precision. That's up to the use case.

I understand why 0 had no suffix here, but I agree that it's a bit too irregular for its own good. It reminds me of ANSI C printf %#x formatting (only) 0 without the 0x prefix, which has caused no end of formatting and parsing surprises. (Go did not adopt this bug.)

@gopherbot
Copy link

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

@rsc rsc changed the title time: print more than just "0" for a zero duration time: print more than just "0" for a zero duration (revert?) May 6, 2016
@rsc rsc reopened this May 6, 2016
@rsc
Copy link
Contributor

rsc commented May 6, 2016

I'm seeing a lot of things break as a result of this change. I'm not convinced it is worth the pain to our users. Even if we can technically do it, it burns some goodwill for not much benefit, since programs will still need to think about older versions that returned "0". Thoughts?

@bradfitz
Copy link
Contributor

bradfitz commented May 6, 2016

I'm in favor of reverting. Similar to locking down testing/quick and continuing to support lax URL parsing, I think we should just accept the old "0" output and not try to fix past mistakes.

@robpike
Copy link
Contributor Author

robpike commented May 6, 2016

The only breaks I've seen are things like golden files for usage messages, which are both unimportant and easy to fix.

I believe that the previous behavior may have been deliberate but it was certainly a bug and should be fixed.

@minux
Copy link
Member

minux commented May 6, 2016 via email

@rsc
Copy link
Contributor

rsc commented May 6, 2016

@robpike, "certainly a bug" is unclear. We all understand the argument for why unitless 0 was used. Whether that's correct is a judgment call. I tend to agree 0s would be preferable. The issue is we might be beyond that point, having used the (defensible) unitless 0 already in seven major Go releases. The question is "is it so wrong, and is the benefit so great, to merit breaking users' programs and burning some goodwill about compatibility?" It's hard for me to see that it is.

For comparison:

  1. In ANSI C %#04x formats 1 as 0x01, but in Go it formats 1 as 0x0001. There is a plausible rationale on both sides, and while it might be nice for Go and ANSI to agree, we decided that was not enough of a benefit to offset the cost to our users of changing the visible behavior of existing Go programs.
  2. Go 1.7 has very significant performance improvements in compress/flate, at the cost of producing different compressed output in certain cases. This affects gzip, zlib, and zip writers in Go. I've already heard from one user who will need to carefully coordinate his system's rolling update from 1.6 to 1.7 because the servers will collectively be giving different outputs. But we justify that pain (and users accept that pain) because of the real benefit of the performance gains.

The 0 vs 0s here seems more like case 1 than case 2.

@robpike
Copy link
Contributor Author

robpike commented May 6, 2016

It's your decision but it's sad if a fix this simple is blocked because of golden tests needing to be updated.

@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 19, 2016
@quentinmit quentinmit assigned rsc and unassigned robpike May 19, 2016
@quentinmit
Copy link
Contributor

Assigning to @rsc since @robpike would like Russ to make a decision.

@rsc
Copy link
Contributor

rsc commented May 27, 2016

From my perspective this change is on probation. We are updating broken tests in Google's sources assuming it will go in, and we'll keep the change in the Go 1.7 beta. So far I don't think we've seen any breakage other than golden files in tests. If that's the only breakage caused, then the change can stay. But if we learn of serious production problems caused by this, we should think strongly about rolling it back.

The saving grace for this change is that time.Duration.String already returns any number of different suffixes in its result, making it a good default for just printing, but typically not nearly consistent enough for use in real production code, where you want to see the same units no matter what, so that different values being printed are comparable (it's too easy to miss the difference between 10m, 10ms, 10us or think that 5.2s > 4m). So there is a chance basically only tests will break.

@rsc
Copy link
Contributor

rsc commented May 27, 2016

Leaving this as Go 1.7 because the decision should be made one way or the other and the bug closed for the release candidate.

@dsnet
Copy link
Member

dsnet commented Jun 6, 2016

Internally, it seems that all breakages are indeed in tests alone and the tests have been corrected to not depend on this behavior. I vote that we keep the behavior (and not revert).

@kkirsche
Copy link
Contributor

kkirsche commented Jun 6, 2016

As someone who uses Go external to Google, I have had no issues with the change in my code which uses time.Duration. For what it's worth, I'd vote to keep the behavior and not revert it.

@adg
Copy link
Contributor

adg commented Jun 6, 2016

Decision: we will not revert the change unless the community encounters significant issues during the beta period.

@adg adg assigned adg and unassigned rsc Jun 6, 2016
@adg
Copy link
Contributor

adg commented Jun 27, 2016

I'm going to close this, as nobody has complained during the beta period.

@adg adg closed this as completed Jun 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests