runtime: dispose gcWork caches before updating controller state

Currently, we only flush the per-P gcWork caches in gcMark, at the
beginning of mark termination. This is necessary to ensure that no
work is held up in these caches.

However, this flush happens after we update the GC controller state,
which depends on statistics about marked heap size and scan work that
are only updated by this flush. Hence, the controller is missing the
bulk of heap marking and scan work. This bug was introduced in commit
1b4025f, which introduced the per-P gcWork caches.

Fix this by flushing these caches before we update the GC controller
state. We continue to flush them at the beginning of mark termination
as well to be robust in case any write barriers happened between the
previous flush and entering mark termination, but this should be a
no-op.

Change-Id: I8f0f91024df967ebf0c616d1c4f0c339c304ebaa
Reviewed-on: https://go-review.googlesource.com/9646
Reviewed-by: Russ Cox <rsc@golang.org>

diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go
index 2c4604da0a13f40276452902810455d35cc5fa4d..fa3573df56487fe2a95db3cd0073c2a1b2f72d8e 100644 (file)
--- a/src/runtime/mgc.go
+++ b/src/runtime/mgc.go
@@ -871,6 +871,11 @@ func gc(mode int) {
                // below. The important thing is that the wb remains active until
                // all marking is complete. This includes writes made by the GC.
 
+               // Flush the gcWork caches. This must be done before
+               // endCycle since endCycle depends on statistics kept
+               // in these caches.
+               gcFlushGCWork()
+
                gcController.endCycle()
        } else {
                // For non-concurrent GC (mode != gcBackgroundMode)
@@ -1163,6 +1168,17 @@ func gcBgMarkDone() {
        }
 }
 
+// gcFlushGCWork disposes the gcWork caches of all Ps. The world must
+// be stopped.
+//go:nowritebarrier
+func gcFlushGCWork() {
+       // Gather all cached GC work. All other Ps are stopped, so
+       // it's safe to manipulate their GC work caches.
+       for i := 0; i < int(gomaxprocs); i++ {
+               allp[i].gcw.dispose()
+       }
+}
+
 // gcMark runs the mark (or, for concurrent GC, mark termination)
 // STW is in effect at this point.
 //TODO go:nowritebarrier
@@ -1179,13 +1195,10 @@ func gcMark(start_time int64) {
 
        gcCopySpans() // TODO(rlh): should this be hoisted and done only once? Right now it is done for normal marking and also for checkmarking.
 
-       // Gather all cached GC work. All other Ps are stopped, so
-       // it's safe to manipulate their GC work caches. During mark
+       // Make sure the per-P gcWork caches are empty. During mark
        // termination, these caches can still be used temporarily,
        // but must be disposed to the global lists immediately.
-       for i := 0; i < int(gomaxprocs); i++ {
-               allp[i].gcw.dispose()
-       }
+       gcFlushGCWork()
 
        work.nwait = 0
        work.ndone = 0