runtime: detangle gcPaceScavenger from the pacer

Currently gcPaceScavenger is called by gcControllerState.commit, but it
manipulates global state which precludes testing. This change detangles
the two.

Change-Id: I10d8ebdf426d99ba49d2f2cb4fb64891e9fd6091
Reviewed-on: https://go-review.googlesource.com/c/go/+/309272
Reviewed-by: Michael Pratt <mpratt@google.com>
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go
index f937287281bc516e4029b7ad432501ecea6132aa..429b9073220fecee8e336944f685e2005bd1d2c0 100644 (file)
--- a/src/runtime/mgc.go
+++ b/src/runtime/mgc.go
@@ -971,6 +971,7 @@ func gcMarkTermination(nextTriggerRatio float64) {
 
        // Update GC trigger and pacing for the next cycle.
        gcController.commit(nextTriggerRatio)
+       gcPaceScavenger(gcController.heapGoal, gcController.lastHeapGoal)
 
        // Update timing memstats
        now := nanotime()

diff --git a/src/runtime/mgcpacer.go b/src/runtime/mgcpacer.go
index 44b870446fece250a60116ec6105454c926f8830..73fe6e15e44a995b9596df5a071fe28090ff7056 100644 (file)
--- a/src/runtime/mgcpacer.go
+++ b/src/runtime/mgcpacer.go
@@ -769,8 +769,6 @@ func (c *gcControllerState) commit(triggerRatio float64) {
                        mheap_.pagesSweptBasis.Store(pagesSwept)
                }
        }
-
-       gcPaceScavenger()
 }
 
 // effectiveGrowthRatio returns the current effective heap growth
@@ -796,6 +794,8 @@ func (c *gcControllerState) effectiveGrowthRatio() float64 {
 // setGCPercent updates gcPercent and all related pacer state.
 // Returns the old value of gcPercent.
 //
+// Calls gcControllerState.commit.
+//
 // The world must be stopped, or mheap_.lock must be held.
 func (c *gcControllerState) setGCPercent(in int32) int32 {
        assertWorldStoppedOrLockHeld(&mheap_.lock)
@@ -819,6 +819,7 @@ func setGCPercent(in int32) (out int32) {
        systemstack(func() {
                lock(&mheap_.lock)
                out = gcController.setGCPercent(in)
+               gcPaceScavenger(gcController.heapGoal, gcController.lastHeapGoal)
                unlock(&mheap_.lock)
        })
 

diff --git a/src/runtime/mgcscavenge.go b/src/runtime/mgcscavenge.go
index 2bb19985db0353aa181f9c40a0e08c6c831fcdeb..fb9b5c86943849b6299e3ebb08cef6658bea68f3 100644 (file)
--- a/src/runtime/mgcscavenge.go
+++ b/src/runtime/mgcscavenge.go
@@ -105,7 +105,8 @@ func heapRetained() uint64 {
 }
 
 // gcPaceScavenger updates the scavenger's pacing, particularly
-// its rate and RSS goal.
+// its rate and RSS goal. For this, it requires the current heapGoal,
+// and the heapGoal for the previous GC cycle.
 //
 // The RSS goal is based on the current heap goal with a small overhead
 // to accommodate non-determinism in the allocator.
@@ -113,18 +114,22 @@ func heapRetained() uint64 {
 // The pacing is based on scavengePageRate, which applies to both regular and
 // huge pages. See that constant for more information.
 //
+// Must be called whenever GC pacing is updated.
+//
 // mheap_.lock must be held or the world must be stopped.
-func gcPaceScavenger() {
+func gcPaceScavenger(heapGoal, lastHeapGoal uint64) {
+       assertWorldStoppedOrLockHeld(&mheap_.lock)
+
        // If we're called before the first GC completed, disable scavenging.
        // We never scavenge before the 2nd GC cycle anyway (we don't have enough
        // information about the heap yet) so this is fine, and avoids a fault
        // or garbage data later.
-       if gcController.lastHeapGoal == 0 {
+       if lastHeapGoal == 0 {
                mheap_.scavengeGoal = ^uint64(0)
                return
        }
        // Compute our scavenging goal.
-       goalRatio := float64(atomic.Load64(&gcController.heapGoal)) / float64(gcController.lastHeapGoal)
+       goalRatio := float64(heapGoal) / float64(lastHeapGoal)
        retainedGoal := uint64(float64(memstats.last_heap_inuse) * goalRatio)
        // Add retainExtraPercent overhead to retainedGoal. This calculation
        // looks strange but the purpose is to arrive at an integer division