From c4931a84332cd9528138651f9c12ab6d63921c68 Mon Sep 17 00:00:00 2001
From: Austin Clements <austin@google.com>
Date: Fri, 1 May 2015 18:08:44 -0400
Subject: [PATCH] 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>
---
 src/runtime/mgc.go | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go
index 2c4604da0a..fa3573df56 100644
--- 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