code review 145010043: runtime: free stacks of Gread goroutines at GC time

runtime: free stacks of Gdead goroutines at GC time

We could probably free the G structures as well, but
for the allg list.  Leaving that for another day.

Fixes issue 8287

[khr, 2014-09-17 00:00:42]

Hello golang-codereviews@googlegroups.com,

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

[dvyukov, 2014-09-17 00:09:03]

https://codereview.appspot.com/145010043/diff/40001/src/runtime/stack.c
File src/runtime/stack.c (right):

https://codereview.appspot.com/145010043/diff/40001/src/runtime/stack.c#newco...
src/runtime/stack.c:807: if(used >= oldsize / 4)
my plan was to pass force argument of gogc down to here
and then relax this condition to oldsize / 2 if force > 0
otherwise we have some lingering per-goroutine memory here as well
can you do this?

[khr1, 2014-09-17 00:20:42]

On Tue, Sep 16, 2014 at 5:09 PM, <dvyukov@google.com> wrote:

>
> https://codereview.appspot.com/145010043/diff/40001/src/runtime/stack.c
> File src/runtime/stack.c (right):
>
> https://codereview.appspot.com/145010043/diff/40001/src/
> runtime/stack.c#newcode807
> src/runtime/stack.c:807: if(used >= oldsize / 4)
> my plan was to pass force argument of gogc down to here
> and then relax this condition to oldsize / 2 if force > 0
> otherwise we have some lingering per-goroutine memory here as well
> can you do this?
>
>
oldsize/2 is a little too strong.  If we shrink at that size, the resulting
stack is completely full.  Maybe oldsize/2 - StackSmall?
In any case, I'm not too worried about this.  The unused space is amortized
over the used space.  I'd like to leave it for 1.5.


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

[dvyukov, 2014-09-17 00:31:37]

On 2014/09/17 00:20:42, khr1 wrote:
> On Tue, Sep 16, 2014 at 5:09 PM, <mailto:dvyukov@google.com> wrote:
> 
> >
> > https://codereview.appspot.com/145010043/diff/40001/src/runtime/stack.c
> > File src/runtime/stack.c (right):
> >
> > https://codereview.appspot.com/145010043/diff/40001/src/
> > runtime/stack.c#newcode807
> > src/runtime/stack.c:807: if(used >= oldsize / 4)
> > my plan was to pass force argument of gogc down to here
> > and then relax this condition to oldsize / 2 if force > 0
> > otherwise we have some lingering per-goroutine memory here as well
> > can you do this?
> >
> >
> oldsize/2 is a little too strong.  If we shrink at that size, the resulting
> stack is completely full.  Maybe oldsize/2 - StackSmall?

But I want to do it only if force>0, which means it is either (1)
debug.FreeOSMemory, in this case it's definitely not too strong or (2) explicit
runtime.GC call, in which case it's probably not too strong as well, because
users are not intended to call it in normal situations, and we do eager sweep
anyway, so it's already more expensive than implicit GC.
Ideally we also need to look at gp.blockedsince and revoke as much space as
possible from goroutines blocked for a long time.


> In any case, I'm not too worried about this.  The unused space is amortized
> over the used space.  I'd like to leave it for 1.5.

It's amortized, but the wastage is up to 75% and stacks can consume lots of
memory.
We can leave it for 1.5, but please file another bug or don't close 8287.

[rsc, 2014-09-17 18:23:13]

LGTM

s/Gread/Gdead/ in CL description

[khr, 2014-09-17 20:25:49]

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

runtime: free stacks of Gdead goroutines at GC time

We could probably free the G structures as well, but
for the allg list.  Leaving that for another day.

Fixes issue 8287

LGTM=rsc
R=golang-codereviews, dvyukov, khr, rsc
CC=golang-codereviews
https://codereview.appspot.com/145010043