code review 155760043: runtime: clear stale values from G.param and SudoG.elem

runtime: clear stale values from G.param and SudoG.elem

This change was necessary on the dev.garbage branch
to keep the garbage collector from seeing pointers into
invalid heap areas.

On this default (Go 1.4) branch, the change removes
some possibility for memory leaks.

[rsc, 2014-10-03 16:27:48]

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

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

[khr, 2014-10-03 17:17:34]

LGTM.

https://codereview.appspot.com/155760043/diff/60001/src/runtime/proc.go
File src/runtime/proc.go (right):

https://codereview.appspot.com/155760043/diff/60001/src/runtime/proc.go#newco...
src/runtime/proc.go:168: if p.elem != nil {
If new is returning non-zeroed memory, we have bigger problems...

[rsc, 2014-10-03 17:32:55]

https://codereview.appspot.com/155760043/diff/60001/src/runtime/proc.go
File src/runtime/proc.go (right):

https://codereview.appspot.com/155760043/diff/60001/src/runtime/proc.go#newco...
src/runtime/proc.go:168: if p.elem != nil {
On 2014/10/03 17:17:34, khr wrote:
> If new is returning non-zeroed memory, we have bigger problems...

Yes, that would be very bad. We were trying to understand why acquireSudog
seemed to be returning one with elem != nil, and it seemed prudent to check all
the code paths. Will remove that one.

[rsc, 2014-10-03 17:36:50]

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

runtime: clear stale values from G.param and SudoG.elem

This change was necessary on the dev.garbage branch
to keep the garbage collector from seeing pointers into
invalid heap areas.

On this default (Go 1.4) branch, the change removes
some possibility for memory leaks.

LGTM=khr
R=golang-codereviews, khr
CC=golang-codereviews, iant, r, rlh
https://codereview.appspot.com/155760043

[gobot, 2014-10-03 23:57:05]

This changed caused perf changes on linux-amd64-perf:


http-4                    old          new      delta
sys-gc                1312851      1247315      -4.99

http-8                    old          new      delta
sys-gc                1841235      1341523     -27.14

http-16                   old          new      delta
sys-gc                2173011      2107475      -3.02

http://build.golang.org/perfdetail?commit=08ccd14a4d7e473f694ed7d748125ccbbea...



—gobot

[gobot, 2014-10-04 00:22:45]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/6d8efcddaccee8532c4984e45e793ea051286865

[dvyukov, 2014-10-04 07:16:31]

> On this default (Go 1.4) branch, the change removes some possibility for
memory leaks.

We clear sudog pool before GC. So GC never scans any free sudog.

[rsc, 2014-10-06 18:47:56]

On Sat, Oct 4, 2014 at 3:16 AM, <dvyukov@google.com> wrote:

> On this default (Go 1.4) branch, the change removes some possibility
>>
> for memory leaks.
>
> We clear sudog pool before GC. So GC never scans any free sudog.
>

Yes it does. There are stale links in G.param. I'm not just fixing things
because I feel like it. I am fixing things because they are broken.

Russ