code review 66830043: runtime: stack allocate Panic structure during runtime.panic

runtime: stack allocate Panic structure during runtime.panic

Update issue 7347

When runtime.panic is called the *Panic is malloced from the heap. This can lead to a gc cycle while panicing which can make a bad situation worse.

It appears to be possible to stack allocate the Panic and avoid malloc'ing during a panic.

Ref: https://groups.google.com/d/topic/golang-dev/OfxqpklGkh0/discussion

[dave_cheney.net, 2014-02-21 06:19:51]

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

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

[minux1, 2014-02-21 06:29:30]

LGTM except for one thing.
You might wait to wait for others.

https://codereview.appspot.com/66830043/diff/40001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/66830043/diff/40001/src/pkg/runtime/panic.c#ne...
src/pkg/runtime/panic.c:220: p.recovered = false;
let's use runtime.memclr for extra safety before initializing the
non-zero fields (in case we add a field to the Panic struct).

[dave_cheney.net, 2014-02-21 06:35:37]

> On 21 Feb 2014, at 17:29, minux.ma@gmail.com wrote:
> 
> LGTM except for one thing.
> You might wait to wait for others.

I've read through the panic methods and I can see they could be recursive, but I
can't see that p could escape. I hope others can confirm/deny this. 

> 
> 
> https://codereview.appspot.com/66830043/diff/40001/src/pkg/runtime/panic.c
> File src/pkg/runtime/panic.c (right):
> 
>
https://codereview.appspot.com/66830043/diff/40001/src/pkg/runtime/panic.c#ne...
> src/pkg/runtime/panic.c:220: p.recovered = false;
> let's use runtime.memclr for extra safety before initializing the
> non-zero fields (in case we add a field to the Panic struct).
> 

Yup, good suggestion, not setting p.recovered causes chaos so memclr is a good
safety net. 

> https://codereview.appspot.com/66830043/

[dave_cheney.net, 2014-02-21 06:55:45]

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

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

[gobot, 2014-02-21 11:36:36]

R=rsc@golang.org (assigned by dave@cheney.net)

[dvyukov, 2014-02-23 11:15:19]

LGTM

[rsc, 2014-02-24 16:09:07]

LGTM

[rsc, 2014-02-24 16:09:23]

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

runtime: stack allocate Panic structure during runtime.panic

Update issue 7347

When runtime.panic is called the *Panic is malloced from the heap. This can lead
to a gc cycle while panicing which can make a bad situation worse.

It appears to be possible to stack allocate the Panic and avoid malloc'ing
during a panic.

Ref: https://groups.google.com/d/topic/golang-dev/OfxqpklGkh0/discussion

LGTM=minux.ma, dvyukov, rsc
R=r, minux.ma, gobot, rsc, dvyukov
CC=golang-codereviews
https://codereview.appspot.com/66830043

Committer: Russ Cox <rsc@golang.org>

[gobot, 2014-02-24 23:23:33]

This CL appears to have broken the windows-amd64 builder.