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

Issue 132960044: code review 132960044: runtime: changes to g->atomicstatus (nee status) to sup... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by rlh
Modified:
9 years, 7 months ago
Reviewers:
gobot, rsc
CC:
golang-codereviews, dvyukov, josharian, khr
Visibility:
Public.

Description

runtime: changes to g->atomicstatus (nee status) to support concurrent GC Every change to g->atomicstatus is now done atomically so that we can ensure that all gs pass through a gc safepoint on demand. This allows the GC to move from one phase to the next safely. In some phases the stack will be scanned. This CL only deals with the infrastructure that allows g->atomicstatus to go from one state to another. Future CLs will deal with scanning and monitoring what phase the GC is in. The major change was to moving to using a Gscan bit to indicate that the status is in a scan state. The only bug fix was in oldstack where I wasn't moving to a Gcopystack state in order to block scanning until the new stack was in place. The proc.go file is waiting for an atomic load instruction.

Patch Set 1 #

Patch Set 2 : diff -r 203e5e3455a0cc7ea42c537eb7c9f60d1dc44c32 https://code.google.com/p/go/ #

Patch Set 3 : diff -r 203e5e3455a0cc7ea42c537eb7c9f60d1dc44c32 https://code.google.com/p/go/ #

Total comments: 144

Patch Set 4 : diff -r 47de34bbcca11cd65f683dc7ab0d1c6f088d25f6 https://code.google.com/p/go/ #

Patch Set 5 : diff -r 47de34bbcca11cd65f683dc7ab0d1c6f088d25f6 https://code.google.com/p/go/ #

Total comments: 59

Patch Set 6 : diff -r 601ad8b625532388ff9dee86409bf32c1f2f624a https://code.google.com/p/go/ #

Patch Set 7 : diff -r 53cc52c4a1dec1ea8ad862af822d1478b24f3d52 https://code.google.com/p/go/ #

Patch Set 8 : diff -r 53cc52c4a1dec1ea8ad862af822d1478b24f3d52 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+309 lines, -88 lines) Patch
M src/pkg/runtime/heapdump.c View 1 2 3 4 chunks +8 lines, -6 lines 0 comments Download
M src/pkg/runtime/mgc0.c View 1 2 3 4 5 7 chunks +11 lines, -8 lines 0 comments Download
M src/pkg/runtime/mprof.goc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/runtime/panic.c View 1 2 3 2 chunks +3 lines, -1 line 0 comments Download
M src/pkg/runtime/proc.c View 1 2 3 4 5 38 chunks +202 lines, -44 lines 0 comments Download
M src/pkg/runtime/proc.go View 1 2 3 4 5 3 chunks +18 lines, -1 line 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 4 5 4 chunks +25 lines, -8 lines 0 comments Download
M src/pkg/runtime/stack.c View 1 2 3 4 5 14 chunks +39 lines, -17 lines 0 comments Download
M src/pkg/runtime/traceback_arm.c View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/runtime/traceback_x86.c View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 24
rlh
Hello golang-codereviews@googlegroups.com (cc: dvyukov@google.com, khr@golang.org, rsc@golang.org), I'd like you to review this change to https://code.google.com/p/go/
9 years, 7 months ago (2014-08-25 20:57:37 UTC) #1
dvyukov
There are lots of inconsistent formatting, unrelated format changes and debug output. I've marked some ...
9 years, 7 months ago (2014-08-26 09:04:33 UTC) #2
dvyukov
I would not rename status to atomicstatus. It would be a mistake to prefix all ...
9 years, 7 months ago (2014-08-26 09:09:08 UTC) #3
rsc
I suggested the rename, and I think it is important. It is not import to ...
9 years, 7 months ago (2014-08-26 14:39:11 UTC) #4
josharian
https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/heapdump.c File src/pkg/runtime/heapdump.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/heapdump.c#newcode459 src/pkg/runtime/heapdump.c:459: runtime·printf("unexpected G.status %d\n", runtime·readgstatus(gp)); There's no clear guarantee that ...
9 years, 7 months ago (2014-08-26 14:55:03 UTC) #5
rsc
lots of comments but overall looks good. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/mgc0.c#newcode761 src/pkg/runtime/mgc0.c:761: runtime·printf(" gp ...
9 years, 7 months ago (2014-08-26 15:27:16 UTC) #6
rlh
PTAL I'm leaving the torture test to run ove night but it has run 5 ...
9 years, 7 months ago (2014-08-26 23:49:14 UTC) #7
rlh
Hello golang-codereviews@googlegroups.com, dvyukov@google.com, josharian@gmail.com, rsc@golang.org (cc: golang-codereviews@googlegroups.com, khr@golang.org), Please take another look.
9 years, 7 months ago (2014-08-26 23:52:52 UTC) #8
rlh
Hello golang-codereviews@googlegroups.com, dvyukov@google.com, josharian@gmail.com, rsc@golang.org (cc: golang-codereviews@googlegroups.com, khr@golang.org), Please take another look.
9 years, 7 months ago (2014-08-26 23:53:26 UTC) #9
rsc
LGTM fix comments as you see fit and submit. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#newcode652 src/pkg/runtime/proc.c:652: ...
9 years, 7 months ago (2014-08-27 11:43:51 UTC) #10
dvyukov
please wait for me as well, I would like to understand how it will work ...
9 years, 7 months ago (2014-08-27 11:54:01 UTC) #11
dvyukov
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#newcode627 src/pkg/runtime/proc.c:627: runtime·casgstatus (G *gp, uint32 oldval, uint32 newval) and I ...
9 years, 7 months ago (2014-08-27 12:11:12 UTC) #12
dvyukov
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#newcode566 src/pkg/runtime/proc.c:566: runtime·casfromgscanstatus (G *gp, uint32 oldval, uint32 newval) remove space ...
9 years, 7 months ago (2014-08-27 13:16:03 UTC) #13
dvyukov
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#newcode605 src/pkg/runtime/proc.c:605: if(newval == Gscanrunning || newval == Gscanenqueue) when the ...
9 years, 7 months ago (2014-08-27 13:37:18 UTC) #14
dvyukov
How are running goroutines scanned? What are transitions between Grunning, Gscanrunning and Gcopystack?
9 years, 7 months ago (2014-08-27 13:38:51 UTC) #15
rsc
Dmitry, the concurrent scan is not in this CL. Please lgtm this and then we ...
9 years, 7 months ago (2014-08-27 14:07:52 UTC) #16
dvyukov
The active spin waits while scanning look like the wrong direction to me. Goroutines must ...
9 years, 7 months ago (2014-08-27 14:09:00 UTC) #17
dvyukov
On 2014/08/27 14:07:52, rsc wrote: > Dmitry, the concurrent scan is not in this CL. ...
9 years, 7 months ago (2014-08-27 14:11:23 UTC) #18
dvyukov
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#newcode602 src/pkg/runtime/proc.c:602: return runtime·cas(&gp->atomicstatus, oldval, newval); A goroutine is Gsyscall status ...
9 years, 7 months ago (2014-08-27 14:30:03 UTC) #19
rlh
Hello golang-codereviews@googlegroups.com, dvyukov@google.com, josharian@gmail.com, rsc@golang.org (cc: golang-codereviews@googlegroups.com, khr@golang.org), Please take another look.
9 years, 7 months ago (2014-08-27 14:39:32 UTC) #20
rsc
LGTM Dmitriy, we can continue this discussion in the next CL or offline. It's a ...
9 years, 7 months ago (2014-08-27 15:06:23 UTC) #21
rlh
*** Submitted as https://code.google.com/p/go/source/detail?r=9fafd63bc3c5 *** runtime: changes to g->atomicstatus (nee status) to support concurrent GC ...
9 years, 7 months ago (2014-08-27 15:15:54 UTC) #22
rsc
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#newcode634 src/pkg/runtime/proc.c:634: while(!runtime·cas(&gp->atomicstatus, oldval, newval)){ On 2014/08/27 15:15:52, rlh wrote: > ...
9 years, 7 months ago (2014-08-27 15:23:45 UTC) #23
gobot
9 years, 7 months ago (2014-08-27 15:40:04 UTC) #24
Message was sent while issue was closed.
This CL appears to have broken the windows-amd64 builder.
See http://build.golang.org/log/595ba451c599fa972e952bcc77a67ed1d16596b9
Sign in to reply to this message.

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