code review 7002049: runtime: earlier detection of unused spans.

runtime: earlier detection of unused spans.

Mark candidate spans one GC pass earlier.

Move scavenger's code out from mgc0 and constrain it into mheap (where it belongs).

[Sebastien Paolacci, 2012-12-22 09:23:52]

Hello golang-dev@googlegroups.com,

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

[rsc, 2013-01-02 22:46:09]

looks good just a few nits

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c
File src/pkg/runtime/mheap.c (right):

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c#newc...
src/pkg/runtime/mheap.c:408: if (trace)
no space between if and (

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c#newc...
src/pkg/runtime/mheap.c:435: runtime·printf("scvg(%d): %D MB released\n", k,
sumreleased>>20);
Please put this print statement back as it was.
sumreleased>>20 is a uintptr, so %p not %D
Also the ( ) are unnecessary.
Let's keep the CL about functionality and not about print nits.

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c#newc...
src/pkg/runtime/mheap.c:436: runtime·printf("scvg(%d): inuse: %D, idle: %D, sys:
%D, released: ~%D -> consumed: ~%D (MB)\n",
Same.

[rsc, 2013-01-02 22:51:57]

please change the CL descrpition to say runtime: not runtime/scavenger:.
(there is no runtime/scavenger package.)

[dvyukov, 2013-01-03 09:46:05]

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c
File src/pkg/runtime/mheap.c (right):

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c#newc...
src/pkg/runtime/mheap.c:334: s->npreleased = t->npreleased;
Is this change correct if we coalesce with both previous and next spans?

[Sebastien Paolacci, 2013-01-06 09:28:04]

Please take another look.

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c
File src/pkg/runtime/mheap.c (right):

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c#newc...
src/pkg/runtime/mheap.c:334: s->npreleased = t->npreleased;
On 2013/01/03 09:46:06, dvyukov wrote:
> Is this change correct if we coalesce with both previous and next spans?
You're right, thanks!
Done.

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c#newc...
src/pkg/runtime/mheap.c:408: if (trace)
On 2013/01/02 22:46:09, rsc wrote:
> no space between if and (

Done.

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c#newc...
src/pkg/runtime/mheap.c:435: runtime·printf("scvg(%d): %D MB released\n", k,
sumreleased>>20);
On 2013/01/02 22:46:09, rsc wrote:
> Please put this print statement back as it was.
> sumreleased>>20 is a uintptr, so %p not %D
It's actually an amount of released pages, which happened to be an uintptr for
the sake of consistency with MSpan's npages, but which should also be displayed
consistently with the just here-below report (and gc's one).
Done.

> Also the ( ) are unnecessary.
> Let's keep the CL about functionality and not about print nits.

https://codereview.appspot.com/7002049/diff/1002/src/pkg/runtime/mheap.c#newc...
src/pkg/runtime/mheap.c:436: runtime·printf("scvg(%d): inuse: %D, idle: %D, sys:
%D, released: ~%D -> consumed: ~%D (MB)\n",
On 2013/01/02 22:46:09, rsc wrote:
> Same.
With the little difference here that accurate accounting can't be achieved by
tracking at the span level (but page) due to merge/split sequences, so it's also
a matter of information correctness.
Same done.

[Sebastien Paolacci, 2013-01-18 21:34:52]

Gentle ping.

[rsc, 2013-01-18 21:48:09]

LGTM

Looks good. Please hg sync + hg mail again once atom's current patch is
submitted (it will change mgc0.c).

[rsc, 2013-01-18 22:39:27]

The patch applied cleanly after all. Submitting.

[Sebastien Paolacci, 2013-01-26 18:47:26]

On 2013/01/18 22:39:27, rsc wrote:
> The patch applied cleanly after all. Submitting.

Hello Russ,

Manually deleted on my side (`hg -D'), but I'm realising that my (synced) repo
is still not cleaned from the CL.

Are you sure it has actually been submitted?

Thanks,
Sebastien

[minux1, 2013-01-26 18:52:14]

On 2013/01/26 18:47:26, Sebastien Paolacci wrote:
> Manually deleted on my side (`hg -D'), but I'm realising that my (synced) repo
> is still not cleaned from the CL.
this CL is not submitted yet, please don't close or delete this CL.

however, you do need to do "hg upload" again as mgc0.c has
got some changes.

[rsc, 2013-01-28 17:53:41]

The patched-in CL was sitting in my client. Submitting now for sure!

[rsc, 2013-01-28 17:53:42]

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

runtime: earlier detection of unused spans.

Mark candidate spans one GC pass earlier.

Move scavenger's code out from mgc0 and constrain it into mheap (where it
belongs).

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

Committer: Russ Cox <rsc@golang.org>

[Sebastien Paolacci, 2013-01-28 22:18:35]

Thanks

On Mon, Jan 28, 2013 at 6:53 PM, Russ Cox <rsc@golang.org> wrote:
> The patched-in CL was sitting in my client. Submitting now for sure!
>

[remyoudompheng, 2013-07-20 20:08:43]

R=close