code review 12053043: runtime: reimplement reflect.call to not use stack spli...

runtime: reimplement reflect.call to not use stack splitting.

[khr, 2013-07-30 17:56:32]

Hello golang-dev@googlegroups.com,

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

[r, 2013-08-01 01:06:19]

those are some big stack frames. what is the right set of realistic sizes?

[khr1, 2013-08-01 03:37:09]

Good question.  I had originally stopped at 64KB, Russ convinced me to go
big :)

We currently have a ~1GB limit on the size of stack frames
(cmd/ld/lib.c:1525), if that's any indication of our willingness to
tolerate crazy code...


On Wed, Jul 31, 2013 at 6:06 PM, <r@golang.org> wrote:

> those are some big stack frames. what is the right set of realistic
> sizes?
>
>
https://codereview.appspot.**com/12053043/<https://codereview.appspot.com/120...
>

[rsc, 2013-08-02 16:36:09]

LGTM

I agree that 1 GB is excessive, but we should stop at something excessive rather
than something just barely implausible. I think 64 kB is too small. 1 MB or 4 MB
is probably a decent limit, but since the sizes are spread exponentially, we're
talking about the difference between 20 or 22 little wrappers and 30 little
wrappers.

https://codereview.appspot.com/12053043/diff/17001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/12053043/diff/17001/src/pkg/reflect/all_test.g...
src/pkg/reflect/all_test.go:3517: if(!testing.Short() && ^uint(0)>>32 != 0) { //
test on 64-bit only
drop ( ) around condition

[khr, 2013-08-02 20:03:18]

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

runtime: reimplement reflect.call to not use stack splitting.

R=golang-dev, r, khr, rsc
CC=golang-dev
https://codereview.appspot.com/12053043

[dave_cheney.net, 2013-08-05 21:51:01]

gentle ping. Would it be possible to temporarily undo this change, or
at least roll back the test so that arm is unbroken.

On Sat, Aug 3, 2013 at 6:03 AM,  <khr@golang.org> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=a5eb738ff649 ***
>
>
> runtime: reimplement reflect.call to not use stack splitting.
>
> R=golang-dev, r, khr, rsc
> CC=golang-dev
> https://codereview.appspot.com/12053043
>
>
>
> https://codereview.appspot.com/12053043/
>
> --
>
> ---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.
>
>

[khr1, 2013-08-05 21:57:38]

Hmmm... TestBigArgs should only fire on 64 bit.  Is that the test you mean?



On Mon, Aug 5, 2013 at 2:50 PM, Dave Cheney <dave@cheney.net> wrote:

> gentle ping. Would it be possible to temporarily undo this change, or
> at least roll back the test so that arm is unbroken.
>
> On Sat, Aug 3, 2013 at 6:03 AM,  <khr@golang.org> wrote:
> > *** Submitted as
> > https://code.google.com/p/go/source/detail?r=a5eb738ff649 ***
> >
> >
> > runtime: reimplement reflect.call to not use stack splitting.
> >
> > R=golang-dev, r, khr, rsc
> > CC=golang-dev
> > https://codereview.appspot.com/12053043
> >
> >
> >
> > https://codereview.appspot.com/12053043/
> >
> > --
> >
> > ---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.
> >
> >
>

[dave_cheney.net, 2013-08-05 21:58:06]

Compiling the test is what causes the crash.
https://codereview.appspot.com/12484043

On Tue, Aug 6, 2013 at 7:57 AM, Keith Randall <khr@google.com> wrote:
> Hmmm... TestBigArgs should only fire on 64 bit.  Is that the test you mean?
>
>
>
> On Mon, Aug 5, 2013 at 2:50 PM, Dave Cheney <dave@cheney.net> wrote:
>>
>> gentle ping. Would it be possible to temporarily undo this change, or
>> at least roll back the test so that arm is unbroken.
>>
>> On Sat, Aug 3, 2013 at 6:03 AM,  <khr@golang.org> wrote:
>> > *** Submitted as
>> > https://code.google.com/p/go/source/detail?r=a5eb738ff649 ***
>> >
>> >
>> > runtime: reimplement reflect.call to not use stack splitting.
>> >
>> > R=golang-dev, r, khr, rsc
>> > CC=golang-dev
>> > https://codereview.appspot.com/12053043
>> >
>> >
>> >
>> > https://codereview.appspot.com/12053043/
>> >
>> > --
>> >
>> > ---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.
>> >
>> >
>
>

[khr1, 2013-08-05 21:59:09]

Ok, I'll kill it.  More hassle than it is worth.


On Mon, Aug 5, 2013 at 2:58 PM, Dave Cheney <dave@cheney.net> wrote:

> Compiling the test is what causes the crash.
> https://codereview.appspot.com/12484043
>
> On Tue, Aug 6, 2013 at 7:57 AM, Keith Randall <khr@google.com> wrote:
> > Hmmm... TestBigArgs should only fire on 64 bit.  Is that the test you
> mean?
> >
> >
> >
> > On Mon, Aug 5, 2013 at 2:50 PM, Dave Cheney <dave@cheney.net> wrote:
> >>
> >> gentle ping. Would it be possible to temporarily undo this change, or
> >> at least roll back the test so that arm is unbroken.
> >>
> >> On Sat, Aug 3, 2013 at 6:03 AM,  <khr@golang.org> wrote:
> >> > *** Submitted as
> >> > https://code.google.com/p/go/source/detail?r=a5eb738ff649 ***
> >> >
> >> >
> >> > runtime: reimplement reflect.call to not use stack splitting.
> >> >
> >> > R=golang-dev, r, khr, rsc
> >> > CC=golang-dev
> >> > https://codereview.appspot.com/12053043
> >> >
> >> >
> >> >
> >> > https://codereview.appspot.com/12053043/
> >> >
> >> > --
> >> >
> >> > ---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.
> >> >
> >> >
> >
> >
>

[dave_cheney.net, 2013-08-06 00:00:18]

Thanks Keith, could you please take a look at
https://code.google.com/p/go/issues/detail?id=6051, it may be related
to the original commit.

On Tue, Aug 6, 2013 at 7:59 AM, Keith Randall <khr@google.com> wrote:
> Ok, I'll kill it.  More hassle than it is worth.
>
>
> On Mon, Aug 5, 2013 at 2:58 PM, Dave Cheney <dave@cheney.net> wrote:
>>
>> Compiling the test is what causes the crash.
>> https://codereview.appspot.com/12484043
>>
>> On Tue, Aug 6, 2013 at 7:57 AM, Keith Randall <khr@google.com> wrote:
>> > Hmmm... TestBigArgs should only fire on 64 bit.  Is that the test you
>> > mean?
>> >
>> >
>> >
>> > On Mon, Aug 5, 2013 at 2:50 PM, Dave Cheney <dave@cheney.net> wrote:
>> >>
>> >> gentle ping. Would it be possible to temporarily undo this change, or
>> >> at least roll back the test so that arm is unbroken.
>> >>
>> >> On Sat, Aug 3, 2013 at 6:03 AM,  <khr@golang.org> wrote:
>> >> > *** Submitted as
>> >> > https://code.google.com/p/go/source/detail?r=a5eb738ff649 ***
>> >> >
>> >> >
>> >> > runtime: reimplement reflect.call to not use stack splitting.
>> >> >
>> >> > R=golang-dev, r, khr, rsc
>> >> > CC=golang-dev
>> >> > https://codereview.appspot.com/12053043
>> >> >
>> >> >
>> >> >
>> >> > https://codereview.appspot.com/12053043/
>> >> >
>> >> > --
>> >> >
>> >> > ---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.
>> >> >
>> >> >
>> >
>> >
>
>

[khr1, 2013-08-06 00:54:52]

Sorry about that, should be fixed for now.


On Mon, Aug 5, 2013 at 5:00 PM, Dave Cheney <dave@cheney.net> wrote:

> Thanks Keith, could you please take a look at
> https://code.google.com/p/go/issues/detail?id=6051, it may be related
> to the original commit.
>
> On Tue, Aug 6, 2013 at 7:59 AM, Keith Randall <khr@google.com> wrote:
> > Ok, I'll kill it.  More hassle than it is worth.
> >
> >
> > On Mon, Aug 5, 2013 at 2:58 PM, Dave Cheney <dave@cheney.net> wrote:
> >>
> >> Compiling the test is what causes the crash.
> >> https://codereview.appspot.com/12484043
> >>
> >> On Tue, Aug 6, 2013 at 7:57 AM, Keith Randall <khr@google.com> wrote:
> >> > Hmmm... TestBigArgs should only fire on 64 bit.  Is that the test you
> >> > mean?
> >> >
> >> >
> >> >
> >> > On Mon, Aug 5, 2013 at 2:50 PM, Dave Cheney <dave@cheney.net> wrote:
> >> >>
> >> >> gentle ping. Would it be possible to temporarily undo this change, or
> >> >> at least roll back the test so that arm is unbroken.
> >> >>
> >> >> On Sat, Aug 3, 2013 at 6:03 AM,  <khr@golang.org> wrote:
> >> >> > *** Submitted as
> >> >> > https://code.google.com/p/go/source/detail?r=a5eb738ff649 ***
> >> >> >
> >> >> >
> >> >> > runtime: reimplement reflect.call to not use stack splitting.
> >> >> >
> >> >> > R=golang-dev, r, khr, rsc
> >> >> > CC=golang-dev
> >> >> > https://codereview.appspot.com/12053043
> >> >> >
> >> >> >
> >> >> >
> >> >> > https://codereview.appspot.com/12053043/
> >> >> >
> >> >> > --
> >> >> >
> >> >> > ---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.
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>

[dave_cheney.net, 2013-08-06 00:58:12]

Thanks for looking at it so quickly. Hopefully we'll get a clean build
across the board this time.

On Tue, Aug 6, 2013 at 10:54 AM, Keith Randall <khr@google.com> wrote:
> Sorry about that, should be fixed for now.
>
>
> On Mon, Aug 5, 2013 at 5:00 PM, Dave Cheney <dave@cheney.net> wrote:
>>
>> Thanks Keith, could you please take a look at
>> https://code.google.com/p/go/issues/detail?id=6051, it may be related
>> to the original commit.
>>
>> On Tue, Aug 6, 2013 at 7:59 AM, Keith Randall <khr@google.com> wrote:
>> > Ok, I'll kill it.  More hassle than it is worth.
>> >
>> >
>> > On Mon, Aug 5, 2013 at 2:58 PM, Dave Cheney <dave@cheney.net> wrote:
>> >>
>> >> Compiling the test is what causes the crash.
>> >> https://codereview.appspot.com/12484043
>> >>
>> >> On Tue, Aug 6, 2013 at 7:57 AM, Keith Randall <khr@google.com> wrote:
>> >> > Hmmm... TestBigArgs should only fire on 64 bit.  Is that the test you
>> >> > mean?
>> >> >
>> >> >
>> >> >
>> >> > On Mon, Aug 5, 2013 at 2:50 PM, Dave Cheney <dave@cheney.net> wrote:
>> >> >>
>> >> >> gentle ping. Would it be possible to temporarily undo this change,
>> >> >> or
>> >> >> at least roll back the test so that arm is unbroken.
>> >> >>
>> >> >> On Sat, Aug 3, 2013 at 6:03 AM,  <khr@golang.org> wrote:
>> >> >> > *** Submitted as
>> >> >> > https://code.google.com/p/go/source/detail?r=a5eb738ff649 ***
>> >> >> >
>> >> >> >
>> >> >> > runtime: reimplement reflect.call to not use stack splitting.
>> >> >> >
>> >> >> > R=golang-dev, r, khr, rsc
>> >> >> > CC=golang-dev
>> >> >> > https://codereview.appspot.com/12053043
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > https://codereview.appspot.com/12053043/
>> >> >> >
>> >> >> > --
>> >> >> >
>> >> >> > ---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.
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>