Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(41)

Issue 6846044: code review 6846044: runtime: use reflect·call() to enter the function gc() (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by atom
Modified:
9 years, 7 months ago
Reviewers:
CC:
rsc, remyoudompheng, dsymonds, minux1, iant, iant2, golang-dev
Visibility:
Public.

Description

runtime: use reflect·call() to enter the function gc() Garbage collection code (to be merged later) is calling functions which have many local variables. This increases the probability that the stack capacity won't be big enough to hold the local variables. So, start gc() on a bigger stack to eliminate a potentially large number of calls to runtime·morestack().

Patch Set 1 #

Patch Set 2 : diff -r fc4e67d82024 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r fc4e67d82024 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 4 : diff -r fc4e67d82024 https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r 9544314de8e8 https://go.googlecode.com/hg/ #

Total comments: 6

Patch Set 6 : diff -r 616adac0bb59 https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r 9050012c9765 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -13 lines) Patch
M src/pkg/runtime/mgc0.c View 1 2 3 4 5 4 chunks +34 lines, -9 lines 0 comments Download
M src/pkg/runtime/proc.c View 4 chunks +12 lines, -4 lines 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 24
atom
Hello rsc, remyoudompheng (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
9 years, 7 months ago (2012-11-13 07:18:42 UTC) #1
dsymonds
Do you have performance numbers?
9 years, 7 months ago (2012-11-13 07:40:56 UTC) #2
atom
On 2012/11/13 07:40:56, dsymonds wrote: > Do you have performance numbers? The probability of stack ...
9 years, 7 months ago (2012-11-13 07:56:45 UTC) #3
dsymonds
On Tue, Nov 13, 2012 at 6:56 PM, <0xE2.0x9A.0x9B@gmail.com> wrote: > On 2012/11/13 07:40:56, dsymonds ...
9 years, 7 months ago (2012-11-13 07:58:42 UTC) #4
atom
On 2012/11/13 07:58:42, dsymonds wrote: > On Tue, Nov 13, 2012 at 6:56 PM, <mailto:0xE2.0x9A.0x9B@gmail.com> ...
9 years, 7 months ago (2012-11-13 09:47:43 UTC) #5
minux1
My benchmark result on linux/amd64 (I've run both the new and old versions 20 times, ...
9 years, 7 months ago (2012-11-13 11:57:16 UTC) #6
atom
On 2012/11/13 11:57:16, minux wrote: > My benchmark result on linux/amd64 (I've run both the ...
9 years, 7 months ago (2012-11-13 12:58:56 UTC) #7
iant
I can't help but feel that this would be better done via some sort of ...
9 years, 7 months ago (2012-11-13 14:28:40 UTC) #8
atom
Hello rsc@golang.org, remyoudompheng@gmail.com, dsymonds@golang.org, minux.ma@gmail.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-13 14:36:34 UTC) #9
atom
https://codereview.appspot.com/6846044/diff/4002/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/6846044/diff/4002/src/pkg/runtime/mgc0.c#newcode1097 src/pkg/runtime/mgc0.c:1097: // a problem in the past. On 2012/11/13 14:28:40, ...
9 years, 7 months ago (2012-11-13 14:37:05 UTC) #10
atom
On 2012/11/13 14:28:40, iant wrote: > I can't help but feel that this would be ...
9 years, 7 months ago (2012-11-13 15:18:33 UTC) #11
iant2
On Tue, Nov 13, 2012 at 7:18 AM, <0xE2.0x9A.0x9B@gmail.com> wrote: > On 2012/11/13 14:28:40, iant ...
9 years, 7 months ago (2012-11-13 16:53:32 UTC) #12
atom
Hello rsc@golang.org, remyoudompheng@gmail.com, dsymonds@golang.org, minux.ma@gmail.com, iant@golang.org, iant@google.com (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-16 07:24:57 UTC) #13
atom
I would like to obtain information about the progress of merging this CL.
9 years, 7 months ago (2012-11-26 09:31:51 UTC) #14
rsc
On Mon, Nov 26, 2012 at 4:31 AM, <0xE2.0x9A.0x9B@gmail.com> wrote: > I would like to ...
9 years, 7 months ago (2012-11-26 14:07:01 UTC) #15
rsc
This seems fine for now. I share Ian's reservations, but this is easy to change ...
9 years, 7 months ago (2012-11-26 20:40:19 UTC) #16
atom
https://codereview.appspot.com/6846044/diff/5/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/6846044/diff/5/src/pkg/runtime/mgc0.c#newcode968 src/pkg/runtime/mgc0.c:968: static void On 2012/11/26 20:40:19, rsc wrote: > Can ...
9 years, 7 months ago (2012-11-27 05:47:20 UTC) #17
atom
Hello rsc@golang.org, remyoudompheng@gmail.com, dsymonds@golang.org, minux.ma@gmail.com, iant@golang.org, iant@google.com (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-27 05:48:00 UTC) #18
rsc
LGTM
9 years, 7 months ago (2012-11-27 14:50:10 UTC) #19
rsc
I tried to patch this in to my tree to submit it, but when I ...
9 years, 7 months ago (2012-11-27 17:20:56 UTC) #20
atom
Hello rsc@golang.org, remyoudompheng@gmail.com, dsymonds@golang.org, minux.ma@gmail.com, iant@golang.org, iant@google.com (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-27 17:53:15 UTC) #21
atom
It appears to be caused by the position of the new field 'moreframesize_minalloc' in struct ...
9 years, 7 months ago (2012-11-27 17:59:32 UTC) #22
rsc
Worked. It's possible that the offset of the tls field is known somewhere deep. I ...
9 years, 7 months ago (2012-11-27 18:04:02 UTC) #23
rsc
9 years, 7 months ago (2012-11-27 18:05:06 UTC) #24
*** Submitted as http://code.google.com/p/go/source/detail?r=7646c94159a1 ***

runtime: use reflect·call() to enter the function gc()

Garbage collection code (to be merged later) is calling functions
which have many local variables. This increases the probability that
the stack capacity won't be big enough to hold the local variables.
So, start gc() on a bigger stack to eliminate a potentially large number
of calls to runtime·morestack().

R=rsc, remyoudompheng, dsymonds, minux.ma, iant, iant
CC=golang-dev
http://codereview.appspot.com/6846044

Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b