runtime: revert to using the precomputed trigger for pacer calculations

Issue #53738 describes in detail how switching to using the actual
trigger point over the precomputed trigger causes a memory regression,
that arises from the fact that the PI controller in front of the
cons/mark ratio has a long time constant (for overdamping), so it
retains a long history of inputs.

This change, for the Go 1.19 cycle, just reverts to using the
precomputed trigger because it's safer, but in the future we should
consider moving away from such a history-sensitive smoothing function.

See the big comment in the diff and #53738 for more details.

Performance difference vs. 1.18 after this change:
https://perf.golang.org/search?q=upload:20220714.15

Fixes #53738.

Change-Id: I636993a730a3eaed25da2a2719860431b296c6f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/417557
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>

diff --git a/src/runtime/mgcpacer.go b/src/runtime/mgcpacer.go
index ac3446db3682fe78949f2d05ae90f0bac918dd82..2d9fd277486ee4025d8be0d3079a6f5201a6f18c 100644 (file)
--- a/src/runtime/mgcpacer.go
+++ b/src/runtime/mgcpacer.go
@@ -439,7 +439,26 @@ func (c *gcControllerState) startCycle(markStartTime int64, procs int, trigger g
        c.fractionalMarkTime = 0
        c.idleMarkTime = 0
        c.markStartTime = markStartTime
-       c.triggered = c.heapLive
+
+       // TODO(mknyszek): This is supposed to be the actual trigger point for the heap, but
+       // causes regressions in memory use. The cause is that the PI controller used to smooth
+       // the cons/mark ratio measurements tends to flail when using the less accurate precomputed
+       // trigger for the cons/mark calculation, and this results in the controller being more
+       // conservative about steady-states it tries to find in the future.
+       //
+       // This conservatism is transient, but these transient states tend to matter for short-lived
+       // programs, especially because the PI controller is overdamped, partially because it is
+       // configured with a relatively large time constant.
+       //
+       // Ultimately, I think this is just two mistakes piled on one another: the choice of a swingy
+       // smoothing function that recalls a fairly long history (due to its overdamped time constant)
+       // coupled with an inaccurate cons/mark calculation. It just so happens this works better
+       // today, and it makes it harder to change things in the future.
+       //
+       // This is described in #53738. Fix this for #53892 by changing back to the actual trigger
+       // point and simplifying the smoothing function.
+       heapTrigger, heapGoal := c.trigger()
+       c.triggered = heapTrigger
 
        // Compute the background mark utilization goal. In general,
        // this may not come out exactly. We round the number of
@@ -501,7 +520,6 @@ func (c *gcControllerState) startCycle(markStartTime int64, procs int, trigger g
        c.revise()
 
        if debug.gcpacertrace > 0 {
-               heapGoal := c.heapGoal()
                assistRatio := c.assistWorkPerByte.Load()
                print("pacer: assist ratio=", assistRatio,
                        " (scan ", gcController.heapScan>>20, " MB in ",