code review 6849097: time: add Now()/UnixNano() malloc tests

time: add Now()/UnixNano() malloc tests

The fix for issue 4403 may include more calls to time.Now().UnixNano(). I was concerned that if this function allocated it would cause additional garbage on the heap. It turns out that it doesn't, which is a nice surprise.

Also add benchmark for Now().UnixNano()

[dfc, 2012-11-24 05:30:24]

Hello bradfitz@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2012-11-24 05:34:08]

LGTM

On Fri, Nov 23, 2012 at 9:30 PM, <dave@cheney.net> wrote:

> Reviewers: bradfitz, minux,
>
> Message:
> Hello bradfitz@golang.org, minux.ma@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> time: add Now()/UnixNano() malloc tests
>
> The fix for issue 4403 may include more calls to time.Now().UnixNano().
> I was concerned that if this function allocated it would cause
> additional garbage on the heap. It turns out that it doesn't, which is a
> nice surprise.
>
> Also add benchmark for Now().UnixNano()
>
> Please review this at
http://codereview.appspot.com/**6849097/<http://codereview.appspot.com/6849097/>
>
> Affected files:
>   M src/pkg/time/time_test.go
>
>
> Index: src/pkg/time/time_test.go
> ==============================**==============================**=======
> --- a/src/pkg/time/time_test.go
> +++ b/src/pkg/time/time_test.go
> @@ -10,6 +10,7 @@
>         "encoding/json"
>         "fmt"
>         "math/rand"
> +       "runtime"
>         "strconv"
>         "strings"
>         "testing"
> @@ -228,7 +229,6 @@
>         {"RFC1123", RFC1123, "Wed, 04 Feb 2009 21:00:57 PST"},
>         {"RFC1123Z", RFC1123Z, "Wed, 04 Feb 2009 21:00:57 -0800"},
>         {"RFC3339", RFC3339, "2009-02-04T21:00:57-08:00"},
> -       {"RFC3339Nano", RFC3339Nano, "2009-02-04T21:00:57.0123456-**
> 08:00"},
>         {"Kitchen", Kitchen, "9:00PM"},
>         {"am/pm", "3pm", "9pm"},
>         {"AM/PM", "3PM", "9PM"},
> @@ -310,6 +310,7 @@
>         {"RFC1123", RFC1123, "Thu, 04 Feb 2010 21:00:57.01234 PST", true,
> true, 1, 5},
>         {"RFC1123Z", RFC1123Z, "Thu, 04 Feb 2010 21:00:57.01234 -0800",
> true, true, 1, 5},
>         {"RFC3339", RFC3339, "2010-02-04T21:00:57.**012345678-08:00",
> true, false, 1, 9},
> +       {"RFC3339Nano", RFC3339Nano, "2010-02-04T21:00:57.**012345678-08:00",
> true, false, 1, 9},
>         {"custom: \"2006-01-02 15:04:05\"", "2006-01-02 15:04:05",
> "2010-02-04 21:00:57.0", false, false, 1, 0},
>         // Amount of white space should not matter.
>         {"ANSIC", ANSIC, "Thu Feb 4 21:00:57 2010", false, true, 1, 0},
> @@ -1037,9 +1038,46 @@
>         }
>  }
>
> +var (
> +       t Time
> +       u int64
> +)
> +
> +var mallocTest = []struct {
> +       count int
> +       desc  string
> +       fn    func()
> +}{
> +       {0, `time.Now()`, func() { t = Now() }},
> +       {0, `time.Now().UnixNano()`, func() { u = Now().UnixNano() }},
> +}
> +
> +func TestCountMallocs(t *testing.T) {
> +       for _, mt := range mallocTest {
> +               const N = 100
> +               memstats := new(runtime.MemStats)
> +               runtime.ReadMemStats(memstats)
> +               mallocs := 0 - memstats.Mallocs
> +               for i := 0; i < N; i++ {
> +                       mt.fn()
> +               }
> +               runtime.ReadMemStats(memstats)
> +               mallocs += memstats.Mallocs
> +               if mallocs/N > uint64(mt.count) {
> +                       t.Errorf("%s: expected %d mallocs, got %d",
> mt.desc, mt.count, mallocs/N)
> +               }
> +       }
> +}
> +
>  func BenchmarkNow(b *testing.B) {
>         for i := 0; i < b.N; i++ {
> -               Now()
> +               t = Now()
> +       }
> +}
> +
> +func BenchmarkNowUnixNano(b *testing.B) {
> +       for i := 0; i < b.N; i++ {
> +               u = Now().UnixNano()
>         }
>  }
>
>
>
>

[dfc, 2012-11-24 05:35:05]

PTAL. I removed some junk that leaked in from a previous change.

[bradfitz, 2012-11-24 05:37:33]

LGTM

On Fri, Nov 23, 2012 at 9:35 PM, <dave@cheney.net> wrote:

> PTAL. I removed some junk that leaked in from a previous change.
>
>
https://codereview.appspot.**com/6849097/<https://codereview.appspot.com/6849...
>

[minux1, 2012-11-24 08:15:37]

tested on windows/386.
FYI: time.Now() will allocate only when stack split occurs.

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

https://codereview.appspot.com/6849097/diff/9001/src/pkg/time/time_test.go#ne...
src/pkg/time/time_test.go:1055: func TestCountMallocs(t *testing.T) {
i think you need to add this:
defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(1))

parallel malloc test is always unstable.

[dfc, 2012-11-24 08:18:44]

Thank you for your comments. I'll submit this when we have a few more builders
passing.

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

https://codereview.appspot.com/6849097/diff/9001/src/pkg/time/time_test.go#ne...
src/pkg/time/time_test.go:1055: func TestCountMallocs(t *testing.T) {
On 2012/11/24 08:15:37, minux wrote:
> i think you need to add this:
> defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(1))
> 
> parallel malloc test is always unstable.

Done.

[dfc, 2012-11-25 00:29:15]

*** Submitted as http://code.google.com/p/go/source/detail?r=50a6b483af08 ***

time: add Now()/UnixNano() malloc tests

The fix for issue 4403 may include more calls to time.Now().UnixNano(). I was
concerned that if this function allocated it would cause additional garbage on
the heap. It turns out that it doesn't, which is a nice surprise.

Also add benchmark for Now().UnixNano()

R=bradfitz, minux.ma
CC=golang-dev
http://codereview.appspot.com/6849097