code review 12946043: runtime: account for all sys memory in MemStats

runtime: account for all sys memory in MemStats
Currently lots of sys allocations are not accounted in any of XxxSys,
including GC bitmap, spans table, GC roots blocks, GC finalizer blocks,
iface table, netpoll descriptors and more. Up to ~20% can unaccounted.
This change introduces 2 new stats: GCSys and OtherSys for GC metadata
and all other misc allocations, respectively.
Also ensures that all XxxSys indeed sum up to Sys. All sys memory allocation
functions require the stat for accounting, so that it's impossible to miss something.
Also fix updating of mcache_sys/inuse, they were not updated after deallocation.

test/bench/garbage/parser before:
Sys		670064344
HeapSys		610271232
StackSys	65536
MSpanSys	14204928
MCacheSys	16384
BuckHashSys	1439992

after:
Sys		670064344
HeapSys		610271232
StackSys	65536
MSpanSys	14188544
MCacheSys	16384
BuckHashSys	3194304
GCSys		39198688
OtherSys	3129656

Fixes issue 5799.

[dvyukov, 2013-08-16 16:24:35]

Hello rsc (cc: golang-dev@googlegroups.com),

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

[dave_cheney.net, 2013-08-17 01:34:52]

This should be documented in the 1.2 notes. 

On 17/08/2013, at 1:24, dvyukov@google.com wrote:

> Reviewers: rsc,
> 
> Message:
> Hello rsc (cc: golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
> 
> 
> Description:
> runtime: account for all sys memory in MemStats
> Currently lots of sys allocations are not accounted in any of XxxSys,
> including GC bitmap, spans table, GC roots blocks, GC finalizer blocks,
> iface table, netpoll descriptors and more. Up to ~20% can unaccounted.
> This change introduces 2 new stats: GCSys and OtherSys for GC metadata
> and all other misc allocations, respectively.
> Also ensures that all XxxSys indeed sum up to Sys. All sys memory
> allocation
> functions require the stat for accounting, so that it's impossible to
> miss something.
> Also fix updating of mcache_sys/inuse, they were not updated after
> deallocation.
> 
> test/bench/garbage/parser before:
> Sys        670064344
> HeapSys        610271232
> StackSys    65536
> MSpanSys    14204928
> MCacheSys    16384
> BuckHashSys    1439992
> 
> after:
> Sys        670064344
> HeapSys        610271232
> StackSys    65536
> MSpanSys    14188544
> MCacheSys    16384
> BuckHashSys    3194304
> GCSys        39198688
> OtherSys    3129656
> 
> Fixes issue 5799.
> 
> Please review this at https://codereview.appspot.com/12946043/
> 
> Affected files:
>  M src/pkg/runtime/cpuprof.c
>  M src/pkg/runtime/iface.c
>  src/pkg/runtime/malloc.goc
>  M src/pkg/runtime/malloc.h
>  M src/pkg/runtime/mem.go
>  M src/pkg/runtime/mem_darwin.c
>  M src/pkg/runtime/mem_freebsd.c
>  M src/pkg/runtime/mem_linux.c
>  M src/pkg/runtime/mem_netbsd.c
>  M src/pkg/runtime/mem_openbsd.c
>  M src/pkg/runtime/mem_plan9.c
>  M src/pkg/runtime/mem_windows.c
>  M src/pkg/runtime/mfixalloc.c
>  M src/pkg/runtime/mgc0.c
>  M src/pkg/runtime/mheap.c
>  M src/pkg/runtime/mprof.goc
>  M src/pkg/runtime/netpoll.goc
>  M src/pkg/runtime/stack.c
> 
> 
> -- 
> 
> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.

[brainman, 2013-08-17 02:18:37]

https://codereview.appspot.com/12946043/diff/23001/src/pkg/runtime/mem_windows.c
File src/pkg/runtime/mem_windows.c (right):

https://codereview.appspot.com/12946043/diff/23001/src/pkg/runtime/mem_window...
src/pkg/runtime/mem_windows.c:28: runtime·xadd64(stat, n);
I think you should add these in SysUnused and SysUsed too. Committed memory is
what OS cares about.

[dvyukov, 2013-08-17 09:34:44]

https://codereview.appspot.com/12946043/diff/23001/src/pkg/runtime/mem_windows.c
File src/pkg/runtime/mem_windows.c (right):

https://codereview.appspot.com/12946043/diff/23001/src/pkg/runtime/mem_window...
src/pkg/runtime/mem_windows.c:28: runtime·xadd64(stat, n);
On 2013/08/17 02:18:37, brainman wrote:
> I think you should add these in SysUnused and SysUsed too. Committed memory is
> what OS cares about.

There is HeapReleased for this.

[brainman, 2013-08-19 00:10:56]

On 2013/08/17 09:34:44, dvyukov wrote:
> ...
> There is HeapReleased for this.

Sure is. Is it possible to add simple test that would break if 

LGTM

I would wait for someone else to review. Is it possible to add test that breaks
if these calculations made wrong?

Alex

[brainman, 2013-08-19 00:11:46]

On 2013/08/19 00:10:56, brainman wrote:
> 
> Sure is. Is it possible to add simple test that would break if 
> 

s/ Is it possible to add simple test that would break if //

Alex

[dvyukov, 2013-08-19 09:17:09]

On 2013/08/19 00:10:56, brainman wrote:
> On 2013/08/17 09:34:44, dvyukov wrote:
> > ...
> > There is HeapReleased for this.
> 
> Sure is. Is it possible to add simple test that would break if 
> 
> LGTM
> 
> I would wait for someone else to review. Is it possible to add test that
breaks
> if these calculations made wrong?

What exactly calculations do you mean? Sys = sum XxxSym?

[dvyukov, 2013-08-19 09:17:26]

On 2013/08/19 09:17:09, dvyukov wrote:
> On 2013/08/19 00:10:56, brainman wrote:
> > On 2013/08/17 09:34:44, dvyukov wrote:
> > > ...
> > > There is HeapReleased for this.
> > 
> > Sure is. Is it possible to add simple test that would break if 
> > 
> > LGTM
> > 
> > I would wait for someone else to review. Is it possible to add test that
> breaks
> > if these calculations made wrong?
> 
> What exactly calculations do you mean? Sys = sum XxxSym?

Sys = sum XxxSys

[brainman, 2013-08-19 09:49:20]

Any calculation will do. That is how you discovered the problem, so put a test
to make sure other won`t break it again.

Alex

[dvyukov, 2013-08-19 19:27:59]

On 2013/08/19 09:49:20, brainman wrote:
> Any calculation will do. That is how you discovered the problem, so put a test
> to make sure other won`t break it again.


Done
see malloc_test.go

[brainman, 2013-08-20 07:37:21]

https://codereview.appspot.com/12946043/diff/35001/src/pkg/runtime/malloc_tes...
File src/pkg/runtime/malloc_test.go (right):

https://codereview.appspot.com/12946043/diff/35001/src/pkg/runtime/malloc_tes...
src/pkg/runtime/malloc_test.go:13: func TestMemStats(t *testing.T) {
Nice. Thank you.

[rsc, 2013-09-06 19:45:16]

LGTM

[rsc, 2013-09-06 20:55:43]

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

runtime: account for all sys memory in MemStats
Currently lots of sys allocations are not accounted in any of XxxSys,
including GC bitmap, spans table, GC roots blocks, GC finalizer blocks,
iface table, netpoll descriptors and more. Up to ~20% can unaccounted.
This change introduces 2 new stats: GCSys and OtherSys for GC metadata
and all other misc allocations, respectively.
Also ensures that all XxxSys indeed sum up to Sys. All sys memory allocation
functions require the stat for accounting, so that it's impossible to miss
something.
Also fix updating of mcache_sys/inuse, they were not updated after deallocation.

test/bench/garbage/parser before:
Sys		670064344
HeapSys		610271232
StackSys	65536
MSpanSys	14204928
MCacheSys	16384
BuckHashSys	1439992

after:
Sys		670064344
HeapSys		610271232
StackSys	65536
MSpanSys	14188544
MCacheSys	16384
BuckHashSys	3194304
GCSys		39198688
OtherSys	3129656

Fixes issue 5799.

R=rsc, dave, alex.brainman
CC=golang-dev
https://codereview.appspot.com/12946043

Committer: Russ Cox <rsc@golang.org>