code review 143150043: runtime: account for tiny allocs, for testing.AllocsPerRun

runtime: account for tiny allocs, for testing.AllocsPerRun

Fixes issue 8734.

[rsc, 2014-09-16 17:44:31]

Hello bradfitz (cc: dvyukov, golang-codereviews@googlegroups.com, iant, khr, r),

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

[r, 2014-09-16 17:47:23]

LGTM

go1.4.text needs to know about this

[bradfitz, 2014-09-16 17:50:59]

LGTM

Thanks!


On Tue, Sep 16, 2014 at 1:44 PM, <rsc@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz (cc: dvyukov, golang-codereviews@googlegroups.com, iant,
> khr, r),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: account for tiny allocs, for testing.AllocsPerRun
>
> Fixes issue 8734.
>
> Please review this at https://codereview.appspot.com/143150043/
>
> Affected files (+12, -3 lines):
>   M src/runtime/malloc.h
>   M src/runtime/malloc.c
>   M src/runtime/malloc.go
>   M src/runtime/mem.go
>   M src/runtime/mheap.c
>   M src/testing/allocs.go
>
>
> Index: src/runtime/malloc.c
> ===================================================================
> --- a/src/runtime/malloc.c
> +++ b/src/runtime/malloc.c
> @@ -79,6 +79,8 @@
>         h = &runtime·mheap;
>         mstats.heap_alloc += c->local_cachealloc;
>         c->local_cachealloc = 0;
> +       mstats.ntinyalloc += c->local_tinyallocs;
> +       c->local_tinyallocs = 0;
>         mstats.nlookup += c->local_nlookup;
>         c->local_nlookup = 0;
>         h->largefree += c->local_largefree;
> Index: src/runtime/malloc.go
> ===================================================================
> --- a/src/runtime/malloc.go
> +++ b/src/runtime/malloc.go
> @@ -103,7 +103,6 @@
>                         // standalone escaping variables. On a json
> benchmark
>                         // the allocator reduces number of allocations by
> ~12% and
>                         // reduces heap size by ~20%.
> -
>                         tinysize := uintptr(c.tinysize)
>                         if size <= tinysize {
>                                 tiny := unsafe.Pointer(c.tiny)
> @@ -121,6 +120,7 @@
>                                         x = tiny
>                                         c.tiny = (*byte)(add(x, size))
>                                         c.tinysize -= uintptr(size1)
> +                                       c.local_tinyallocs++
>                                         if debugMalloc {
>                                                 mp := acquirem()
>                                                 if mp.mallocing == 0 {
> Index: src/runtime/malloc.h
> ===================================================================
> --- a/src/runtime/malloc.h
> +++ b/src/runtime/malloc.h
> @@ -239,6 +239,7 @@
>         uint64  sys;            // bytes obtained from system (should be
> sum of xxx_sys below, no locking, approximate)
>         uint64  nlookup;        // number of pointer lookups
>         uint64  nmalloc;        // number of mallocs
> +       uint64  ntinyalloc;     // number of tinyallocs that didn't malloc
>         uint64  nfree;  // number of frees
>
>         // Statistics about malloc heap.
> @@ -331,6 +332,7 @@
>         // See "Tiny allocator" comment in malloc.goc.
>         byte*   tiny;
>         uintptr tinysize;
> +       uintptr local_tinyallocs;       // number of tiny allocs not
> counted in other stats
>         // The rest is not accessed on every malloc.
>         MSpan*  alloc[NumSizeClasses];  // spans to allocate from
>
> Index: src/runtime/mem.go
> ===================================================================
> --- a/src/runtime/mem.go
> +++ b/src/runtime/mem.go
> @@ -17,6 +17,7 @@
>         Sys        uint64 // bytes obtained from system (sum of XxxSys
> below)
>         Lookups    uint64 // number of pointer lookups
>         Mallocs    uint64 // number of mallocs
> +       TinyAllocs uint64 // number of tiny allocations merged into
> others; not counted in Mallocs
>         Frees      uint64 // number of frees
>
>         // Main allocation heap statistics.
> Index: src/runtime/mheap.c
> ===================================================================
> --- a/src/runtime/mheap.c
> +++ b/src/runtime/mheap.c
> @@ -184,6 +184,8 @@
>         // transfer stats from cache to global
>         mstats.heap_alloc += g->m->mcache->local_cachealloc;
>         g->m->mcache->local_cachealloc = 0;
> +       mstats.ntinyalloc += g->m->mcache->local_tinyallocs;
> +       g->m->mcache->local_tinyallocs = 0;
>
>         s = MHeap_AllocSpanLocked(h, npage);
>         if(s != nil) {
> @@ -465,6 +467,8 @@
>         runtime·lock(&h->lock);
>         mstats.heap_alloc += g->m->mcache->local_cachealloc;
>         g->m->mcache->local_cachealloc = 0;
> +       mstats.ntinyalloc += g->m->mcache->local_tinyallocs;
> +       g->m->mcache->local_tinyallocs = 0;
>         if(acct) {
>                 mstats.heap_alloc -= s->npages<<PageShift;
>                 mstats.heap_objects--;
> Index: src/testing/allocs.go
> ===================================================================
> --- a/src/testing/allocs.go
> +++ b/src/testing/allocs.go
> @@ -26,7 +26,7 @@
>         // Measure the starting statistics
>         var memstats runtime.MemStats
>         runtime.ReadMemStats(&memstats)
> -       mallocs := 0 - memstats.Mallocs
> +       mallocs := 0 - (memstats.Mallocs + memstats.TinyAllocs)
>
>         // Run the function the specified number of times
>         for i := 0; i < runs; i++ {
> @@ -35,7 +35,7 @@
>
>         // Read the final statistics
>         runtime.ReadMemStats(&memstats)
> -       mallocs += memstats.Mallocs
> +       mallocs += memstats.Mallocs + memstats.TinyAllocs
>
>         // Average the mallocs over the runs (not counting the warm-up).
>         // We are forced to return a float64 because the API is silly, but
> do
>
>
>

[rsc, 2014-09-16 21:41:20]

dmitriy, should i wait for you?

[dvyukov, 2014-09-16 22:08:27]

https://codereview.appspot.com/143150043/diff/40001/src/runtime/mem.go
File src/runtime/mem.go (right):

https://codereview.appspot.com/143150043/diff/40001/src/runtime/mem.go#newcode20
src/runtime/mem.go:20: TinyAllocs uint64 // number of tiny allocations merged
into others; not counted in Mallocs
Normal mallocs are called Malloc and this is Allocs, is it deliberate? Should
this be TinyMallocs?

[dvyukov, 2014-09-16 22:15:07]

https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go
File src/runtime/malloc.go (right):

https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go#new...
src/runtime/malloc.go:123: c.local_tinyallocs++
here is a slight potential for overflow
if you allocate zillions of 1 byte objects, then you need only 256MB to overflow
32-bit uintptr, this can happen w/o triggering GC nor going into mheap for new
spans (if we obtain partially filled spans form mcentral)

[rsc, 2014-09-16 22:56:41]

https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go
File src/runtime/malloc.go (right):

https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go#new...
src/runtime/malloc.go:123: c.local_tinyallocs++
On 2014/09/16 22:15:07, dvyukov wrote:
> here is a slight potential for overflow
> if you allocate zillions of 1 byte objects, then you need only 256MB to
overflow
> 32-bit uintptr, this can happen w/o triggering GC nor going into mheap for new
> spans (if we obtain partially filled spans form mcentral)

I'm sorry but I don't understand. 
If you only have 256 MB then it seems like you can only do 256M 1-byte
tinyallocs with it.

More generally, local_cachealloc is only an intptr and it seems like (excluding
the subtraction during garbage collection) it must be strictly greater than
local_tinyallocs, so a uintptr should be fine.

[dvyukov, 2014-09-16 23:26:49]

LGTM

https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go
File src/runtime/malloc.go (right):

https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go#new...
src/runtime/malloc.go:123: c.local_tinyallocs++
On 2014/09/16 22:56:41, rsc wrote:
> On 2014/09/16 22:15:07, dvyukov wrote:
> > here is a slight potential for overflow
> > if you allocate zillions of 1 byte objects, then you need only 256MB to
> overflow
> > 32-bit uintptr, this can happen w/o triggering GC nor going into mheap for
new
> > spans (if we obtain partially filled spans form mcentral)
> 
> I'm sorry but I don't understand. 
> If you only have 256 MB then it seems like you can only do 256M 1-byte
> tinyallocs with it.
> 
> More generally, local_cachealloc is only an intptr and it seems like
(excluding
> the subtraction during garbage collection) it must be strictly greater than
> local_tinyallocs, so a uintptr should be fine.


you are right
ignore this comment

[rsc, 2014-09-17 00:33:00]

I renamed TinyAllocs to TinyMallocs, but that got me thinking. One thing we
could do is add the amount for TinyAllocs to both Mallocs and Frees. Then
Mallocs goes up for testing.AllocsPerRun and other users, without needing
to export a new field, and Mallocs - Frees continues to be meaningful.

I'm leaning toward doing that. Thoughts?

Russ

[dvyukov, 2014-09-17 00:35:23]

sounds good to me

On Tue, Sep 16, 2014 at 5:33 PM, Russ Cox <rsc@golang.org> wrote:
> I renamed TinyAllocs to TinyMallocs, but that got me thinking. One thing we
> could do is add the amount for TinyAllocs to both Mallocs and Frees. Then
> Mallocs goes up for testing.AllocsPerRun and other users, without needing to
> export a new field, and Mallocs - Frees continues to be meaningful.
>
> I'm leaning toward doing that. Thoughts?
>
> Russ
>
>

[bradfitz, 2014-09-17 02:27:10]

Did this just go full circle? Wasn't that the original idea?


On Tue, Sep 16, 2014 at 8:35 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> sounds good to me
>
> On Tue, Sep 16, 2014 at 5:33 PM, Russ Cox <rsc@golang.org> wrote:
> > I renamed TinyAllocs to TinyMallocs, but that got me thinking. One thing
> we
> > could do is add the amount for TinyAllocs to both Mallocs and Frees. Then
> > Mallocs goes up for testing.AllocsPerRun and other users, without
> needing to
> > export a new field, and Mallocs - Frees continues to be meaningful.
> >
> > I'm leaning toward doing that. Thoughts?
> >
> > Russ
> >
> >
>

[rsc, 2014-09-17 18:49:39]

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

runtime: account for tiny allocs, for testing.AllocsPerRun

Fixes issue 8734.

LGTM=r, bradfitz, dvyukov
R=bradfitz, r, dvyukov
CC=golang-codereviews, iant, khr
https://codereview.appspot.com/143150043