code review 7135063: runtime: local allocation in mprof.goc

runtime: local allocation in mprof.goc

Binary data in mprof.goc may prevent the garbage collector from freeing
memory blocks. This patch replaces all calls to runtime·mallocgc() with
calls to an allocator private to mprof.goc, thus making the private
memory invisible to the garbage collector. The addrhash variable is
moved outside of the .bss section.

[atom, 2013-01-19 12:58:29]

Hello golang-dev@googlegroups.com (cc: rsc, dvyukov, dfc, minux,
remyoudompheng),

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

[atom, 2013-01-19 13:10:37]

On 2013/01/19 12:58:29, atom wrote:
> Hello mailto:golang-dev@googlegroups.com (cc: rsc, dvyukov, dfc, minux,
> remyoudompheng),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

mgc0.c will also call the function markonly() as part of precise garbage
collection of hashmaps which will be submitted in a next CL.

While markonly() contains redundant code, it seems the function cannot be
removed or simplified. CL 6114046 implements a suboptimal (performance-wise)
garbage collection of hashmaps, so the function markonly() may be removed or
adjusted as part of a faster GC of hashmaps.

[dvyukov, 2013-01-20 10:00:06]

Perhaps it will be simpler to allocate all mprof-related memory from Sys
regions, then they will be automatically not scanned. No need for any special
marking, etc.

https://codereview.appspot.com/7135063/diff/1/src/pkg/runtime/mprof.goc
File src/pkg/runtime/mprof.goc (right):

https://codereview.appspot.com/7135063/diff/1/src/pkg/runtime/mprof.goc#newco...
src/pkg/runtime/mprof.goc:518: for(e=addrfree; e; e=e->next) {
all objects in addrfree list are allocated with single malloc() call, so you
must not mark them all

[atom, 2013-01-20 10:24:25]

https://codereview.appspot.com/7135063/diff/1/src/pkg/runtime/mprof.goc
File src/pkg/runtime/mprof.goc (right):

https://codereview.appspot.com/7135063/diff/1/src/pkg/runtime/mprof.goc#newco...
src/pkg/runtime/mprof.goc:518: for(e=addrfree; e; e=e->next) {
On 2013/01/20 10:00:06, dvyukov wrote:
> all objects in addrfree list are allocated with single malloc() call,

Not true.

> so you must not mark them all

It is safe to call runtime·markobject() multiple times with arguments pointing
to the same block of memory.

[atom, 2013-01-20 10:58:42]

On 2013/01/20 10:00:06, dvyukov wrote:
> Perhaps it will be simpler to allocate all mprof-related memory from Sys
> regions, then they will be automatically not scanned. No need for any special
> marking, etc.

If we will agree upon that this does not constrain the future development
(whatever it will be) of mprof.goc then I will reimplement this CL with calls to
runtime·SysAlloc() and without any calls to mallocgc().

[dvyukov, 2013-01-21 06:13:08]

As far as I see google heap profiler frees memory only during
stop/shutdown (here we can unmap all mapped memory at once) or for
temp storage (e.g. snapshots, here we can use normal heap memory since
it's transient):
http://code.google.com/p/gperftools/source/browse/trunk/src/heap-profiler.cc?...
http://code.google.com/p/gperftools/source/browse/trunk/src/heap-profile-tabl...

On Sun, Jan 20, 2013 at 2:58 PM,  <0xE2.0x9A.0x9B@gmail.com> wrote:
> On 2013/01/20 10:00:06, dvyukov wrote:
>>
>> Perhaps it will be simpler to allocate all mprof-related memory from
>
> Sys
>>
>> regions, then they will be automatically not scanned. No need for any
>
> special
>>
>> marking, etc.
>
>
> If we will agree upon that this does not constrain the future
> development (whatever it will be) of mprof.goc then I will reimplement
> this CL with calls to runtime·SysAlloc() and without any calls to
> mallocgc().
>
> https://codereview.appspot.com/7135063/

[atom, 2013-01-22 14:26:56]

Hello golang-dev@googlegroups.com, dvyukov@google.com (cc: dave@cheney.net,
golang-dev@googlegroups.com, minux.ma@gmail.com, remyoudompheng@gmail.com,
rsc@golang.org),

Please take another look.

[atom, 2013-01-22 14:30:36]

On 2013/01/22 14:26:56, atom wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:dvyukov@google.com (cc:
mailto:dave@cheney.net,
> mailto:golang-dev@googlegroups.com, mailto:minux.ma@gmail.com,
mailto:remyoudompheng@gmail.com,
> mailto:rsc@golang.org),
> 
> Please take another look.

Should the code also update mstats?

[rsc, 2013-01-23 04:44:19]

LGTM

https://codereview.appspot.com/7135063/diff/12001/src/pkg/runtime/mprof.goc
File src/pkg/runtime/mprof.goc (right):

https://codereview.appspot.com/7135063/diff/12001/src/pkg/runtime/mprof.goc#n...
src/pkg/runtime/mprof.goc:30: allocate(uintptr size) {
brace on next line please

[rsc, 2013-01-23 04:45:27]

Regarding mstats, SysAlloc will already bump mstats.sys. I'd rather not add
a new field to mstat until we know it's necessary.

[atom, 2013-01-23 07:16:55]

Hello golang-dev@googlegroups.com, dvyukov@google.com, rsc@golang.org (cc:
dave@cheney.net, golang-dev@googlegroups.com, minux.ma@gmail.com,
remyoudompheng@gmail.com),

Please take another look.

[atom, 2013-01-23 07:19:00]

On 2013/01/23 04:45:27, rsc wrote:
> Regarding mstats, SysAlloc will already bump mstats.sys. I'd rather not add
> a new field to mstat until we know it's necessary.

http://golang.org/pkg/runtime/#MemStats says that "should be sum of XxxSys
below". The sum is already off by an amount.

[dvyukov, 2013-01-23 07:21:13]

On 2013/01/23 07:19:00, atom wrote:
> On 2013/01/23 04:45:27, rsc wrote:
> > Regarding mstats, SysAlloc will already bump mstats.sys. I'd rather not add
> > a new field to mstat until we know it's necessary.
> 
> http://golang.org/pkg/runtime/#MemStats says that "should be sum of XxxSys
> below". The sum is already off by an amount.

I think there are already other such cases, e.g. GC allocates some Sys memory.

[dvyukov, 2013-01-23 07:23:09]

LGTM

[minux1, 2013-01-23 08:22:34]

https://codereview.appspot.com/7135063/diff/18001/src/pkg/runtime/mprof.goc
File src/pkg/runtime/mprof.goc (right):

https://codereview.appspot.com/7135063/diff/18001/src/pkg/runtime/mprof.goc#n...
src/pkg/runtime/mprof.goc:19: // The profiler is forbidden from refering to
garbage-collected memory.
s/refering/referring/

https://codereview.appspot.com/7135063/diff/18001/src/pkg/runtime/mprof.goc#n...
src/pkg/runtime/mprof.goc:38: return runtime·SysAlloc(size);
perhaps you need to check for nil?

[atom, 2013-01-23 08:46:16]

Hello golang-dev@googlegroups.com, dvyukov@google.com, rsc@golang.org,
minux.ma@gmail.com (cc: dave@cheney.net, golang-dev@googlegroups.com,
remyoudompheng@gmail.com),

Please take another look.

[atom, 2013-01-23 08:47:53]

https://codereview.appspot.com/7135063/diff/18001/src/pkg/runtime/mprof.goc
File src/pkg/runtime/mprof.goc (right):

https://codereview.appspot.com/7135063/diff/18001/src/pkg/runtime/mprof.goc#n...
src/pkg/runtime/mprof.goc:19: // The profiler is forbidden from refering to
garbage-collected memory.
On 2013/01/23 08:22:34, minux wrote:
> s/refering/referring/

Done.

https://codereview.appspot.com/7135063/diff/18001/src/pkg/runtime/mprof.goc#n...
src/pkg/runtime/mprof.goc:38: return runtime·SysAlloc(size);
On 2013/01/23 08:22:34, minux wrote:
> perhaps you need to check for nil?

There are multiple places in package runtime where this chech is omitted. I
suggest to submit this in a separate CL.

[minux1, 2013-01-23 09:07:22]

LGTM.

https://codereview.appspot.com/7135063/diff/18001/src/pkg/runtime/mprof.goc
File src/pkg/runtime/mprof.goc (right):

https://codereview.appspot.com/7135063/diff/18001/src/pkg/runtime/mprof.goc#n...
src/pkg/runtime/mprof.goc:38: return runtime·SysAlloc(size);
On 2013/01/23 08:47:53, atom wrote:
> On 2013/01/23 08:22:34, minux wrote:
> > perhaps you need to check for nil?
> 
> There are multiple places in package runtime where this chech is omitted. I
> suggest to submit this in a separate CL.
ok. we need a thorough review of existing code for this problem.

[rsc, 2013-01-30 17:01:04]

LGTM

[rsc, 2013-01-30 17:01:34]

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

runtime: local allocation in mprof.goc

Binary data in mprof.goc may prevent the garbage collector from freeing
memory blocks. This patch replaces all calls to runtime·mallocgc() with
calls to an allocator private to mprof.goc, thus making the private
memory invisible to the garbage collector. The addrhash variable is
moved outside of the .bss section.

R=golang-dev, dvyukov, rsc, minux.ma
CC=dave, golang-dev, remyoudompheng
https://codereview.appspot.com/7135063

Committer: Russ Cox <rsc@golang.org>