code review 6903050: time: add Round and Truncate

time: add Round and Truncate

New in Go 1 will be nanosecond precision in the result of time.Now on Linux.
This will break code that stores time in external formats at microsecond
precision, reads it back, and expects to get exactly the same time.

Code like that can be fixed by using time.Now().Round(time.Microsecond)
instead of time.Now() in those contexts.

[rsc, 2012-12-07 18:35:19]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-12-07 22:26:09]

Rob signed off on the API but I'd like someone to review the code,
especially the math. Maybe Rémy or Robert?

Thanks.
Russ

[bradfitz, 2012-12-07 22:36:00]

s/precision/resolution/g?

On Fri, Dec 7, 2012 at 10:35 AM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> time: add Round and Truncate
>
> New in Go 1 will be nanosecond precision in the result of time.Now on
> Linux.
> This will break code that stores time in external formats at microsecond
> precision, reads it back, and expects to get exactly the same time.
>
> Code like that can be fixed by using time.Now().Round(time.**Microsecond)
> instead of time.Now() in those contexts.
>
> Please review this at
https://codereview.appspot.**com/6903050/<https://codereview.appspot.com/6903...
>
> Affected files:
>   M doc/go1.1.html
>   M src/pkg/time/example_test.go
>   M src/pkg/time/time.go
>   M src/pkg/time/time_test.go
>
>
>

[iant, 2012-12-08 08:35:52]

LGTM

[remyoudompheng, 2012-12-08 09:51:50]

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1037
src/pkg/time/time.go:1037: // Truncate returns the result of rounding t down to
a multiple of d.
(see below: "Time t is a multiple of Duration d" doesn't mean anything to me).

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1065
src/pkg/time/time.go:1065: return int(t.nsec/int32(d)) & 1, Duration(t.nsec %
int32(d))
this is not correct, for example if d == Second. In this case I would expect to
see t.sec&1 affect qmod2, but the code only mentions t.nsec.

This code is correct if 2*d divides Second.

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1071
src/pkg/time/time.go:1071: return int(t.sec/d1) & 1, Duration(t.sec%d1)*Second +
Duration(t.nsec)
I think t.sec/d1 is not what you want in case t.sec < 0 (which is highly
unlikely anyway). The comment for div should explain whether the returned r is
always >= 0.

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1078
src/pkg/time/time.go:1078: 
I think rounding time for a duration which is not a divisor of 86400*time.Second
or of time.Hour doesn't make sense because the result will depend on which day
is chosen as origin of time, so it would be essentially unspecified.

But rounding to 360ms (which is time.Hour / 10000) can make sense, and is not is
the previous easy cases.

Rounding a time.Time to 7 seconds (t.Truncate(7*time.Second)) and a Unix time to
7 seconds (for example, u := t.Unix(); t = time.Unix(u-u%7, 0)) will give
different results even for reasonable values of t, and this looks very
confusing.

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1080
src/pkg/time/time.go:1080: tmp := (uint64(t.sec) >> 32) * 1e9
I'm not sure this makes any sense if t.sec < 0.

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1082
src/pkg/time/time.go:1082: u0 := tmp << 32
if the goal is u1<<64|u0 = (tmp&^0xffffffff)*1e9 here, I think u0 should be
(tmp&0xffffffff) << 32.

I'm not yet in shape to look at the things below.

[remyoudompheng, 2012-12-08 09:56:07]

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/example_test.go
File src/pkg/time/example_test.go (right):

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/example_test.go#ne...
src/pkg/time/example_test.go:60: func ExampleTime_Round() {
there should be a test demonstrating the round-to-even behaviour in very simple
cases (which I think are broken, see previuos comment).

Parse("2012 Dec 07 12:15:30.5").Round(time.Second) -> 12:15:30
Parse("2012 Dec 07 12:15:31.5").Round(time.Second) -> 12:15:32

[remyoudompheng, 2012-12-08 23:01:16]

Another example of how "round-even" may have an unclear meaning. Suppose I am
calling t.Round(time.Hour). I expect to get an even number of hours if I'm
halfway. But:

- if t is 16:30:00 UTC, it gives 16:00:00 UTC
- if t is 16:30:00 CET (UTC+1), it gives 17:00:00 CET ?
- if t is 16:30:00 CEST (UTC+2), it gives 16:00:00 CEST ?

Is it something a sane user can expect?

Rémy.

[rsc, 2012-12-09 06:58:34]

On 2012/12/08 23:01:16, remyoudompheng wrote:
> Another example of how "round-even" may have an unclear meaning. Suppose I am
> calling t.Round(time.Hour). I expect to get an even number of hours if I'm
> halfway. But:
> 
> - if t is 16:30:00 UTC, it gives 16:00:00 UTC
> - if t is 16:30:00 CET (UTC+1), it gives 17:00:00 CET ?
> - if t is 16:30:00 CEST (UTC+2), it gives 16:00:00 CEST ?
> 
> Is it something a sane user can expect?

Yeah, that's annoying. It's probably better to round up always, which I've done
now.
Note that if you are in a fractional time zone that is n hours + 30 minutes off
from UTC,
rounding to hour produces times ending in :30 in the local zone.
Similarly, in any non-UTC local zone, rounding to 1*Day does not produce
00:00:00.

These are all implications of using the zero time instant as zero.
I think it is better to live with them than to make the computation
location-dependent.

[rsc, 2012-12-09 06:59:09]

@bradfitz: we use precision elsewhere in the package instead of resolution

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1037
src/pkg/time/time.go:1037: // Truncate returns the result of rounding t down to
a multiple of d.
On 2012/12/08 09:51:50, remyoudompheng wrote:
> (see below: "Time t is a multiple of Duration d" doesn't mean anything to me).

I understand what you're saying, but I don't have a good way to
describe it that doesn't cause more confusion than it resolves. 
For cases people will use it for - like Truncate(time.Millisecond),
the choice of zero is irrelevant, and I want to avoid making people
think zero matters more than it does.

Changed to:

// Truncate returns the result of rounding t down to a multiple of d (since the
zero time).

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1065
src/pkg/time/time.go:1065: return int(t.nsec/int32(d)) & 1, Duration(t.nsec %
int32(d))
On 2012/12/08 09:51:50, remyoudompheng wrote:
> this is not correct, for example if d == Second. In this case I would expect
to
> see t.sec&1 affect qmod2, but the code only mentions t.nsec.
> 
> This code is correct if 2*d divides Second.

Nice catch, thanks. Made that the condition.

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1071
src/pkg/time/time.go:1071: return int(t.sec/d1) & 1, Duration(t.sec%d1)*Second +
Duration(t.nsec)
On 2012/12/08 09:51:50, remyoudompheng wrote:
> I think t.sec/d1 is not what you want in case t.sec < 0 (which is highly
> unlikely anyway). The comment for div should explain whether the returned r is
> always >= 0.

You're right. I forgot we allowed t.sec >= 0. Fixed this whole routine to handle
that.

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1078
src/pkg/time/time.go:1078: 
On 2012/12/08 09:51:50, remyoudompheng wrote:
> I think rounding time for a duration which is not a divisor of
86400*time.Second
> or of time.Hour doesn't make sense because the result will depend on which day
> is chosen as origin of time, so it would be essentially unspecified.

Agreed until the last word. We've defined where 0 (the zero Time) is, so it's
all specified. I agree it's a useless thing to do, but it's all specified, and
if asked to round to 3 nanoseconds or 3 days, I'd rather give a consistent
result than panic.

> Rounding a time.Time to 7 seconds (t.Truncate(7*time.Second)) and a Unix time
to
> 7 seconds (for example, u := t.Unix(); t = time.Unix(u-u%7, 0)) will give
> different results even for reasonable values of t, and this looks very
> confusing.

It depends. If you actually notice that it's not where you think it is, you're
looking _very_ close and will probably figure out where 0 is. On the other hand,
if you just want to do something every 7 seconds and want a consistent way to
generate times 7 seconds apart, this works fine.

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1080
src/pkg/time/time.go:1080: tmp := (uint64(t.sec) >> 32) * 1e9
On 2012/12/08 09:51:50, remyoudompheng wrote:
> I'm not sure this makes any sense if t.sec < 0.

Fixed.

https://codereview.appspot.com/6903050/diff/3/src/pkg/time/time.go#newcode1082
src/pkg/time/time.go:1082: u0 := tmp << 32
On 2012/12/08 09:51:50, remyoudompheng wrote:
> if the goal is u1<<64|u0 = (tmp&^0xffffffff)*1e9 here, I think u0 should be
> (tmp&0xffffffff) << 32.

tmp<<32 and (tmp&0xffffffff)<<32 are the same thing, no?

> I'm not yet in shape to look at the things below.

No worries. Thanks. The test against math/big makes me pretty confident.

[rsc, 2012-12-09 07:00:06]

PTAL rémy

[remyoudompheng, 2012-12-09 08:24:07]

LGTM after the test fix

https://codereview.appspot.com/6903050/diff/4/src/pkg/time/time_test.go
File src/pkg/time/time_test.go (right):

https://codereview.appspot.com/6903050/diff/4/src/pkg/time/time_test.go#newco...
src/pkg/time/time_test.go:340: for Second%d == 0 {
i think your condition is reversed. You want to stop when Second%d == 0. But if
d was 500000001 you are really, really screwed, and even more on ARM. I suggest:

      // divisors of Second
      f1 := func(ti int64, tns int32, logdi uint32) bool {
            d := Duration(1)
            a, b := logdi%9, (logdi>>16)%9
            d <<= a
            for i := 0; i < int(b); i++ {
                  d *= 5
            }
            return testOne(ti, int64(tns), int64(d))
      }
      quick.Check(f1, cfg)

[rsc, 2012-12-09 08:59:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=ea15e7ed6d72 ***

time: add Round and Truncate

New in Go 1 will be nanosecond precision in the result of time.Now on Linux.
This will break code that stores time in external formats at microsecond
precision, reads it back, and expects to get exactly the same time.

Code like that can be fixed by using time.Now().Round(time.Microsecond)
instead of time.Now() in those contexts.

R=golang-dev, bradfitz, iant, remyoudompheng
CC=golang-dev
https://codereview.appspot.com/6903050