From 85a482fc244f6f118b1d063063a51eb8b0feadd8 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Thu, 14 Jul 2022 21:19:37 +0000 Subject: [PATCH] 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 Reviewed-by: Michael Pratt Run-TryBot: Michael Knyszek --- src/runtime/mgcpacer.go | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/runtime/mgcpacer.go b/src/runtime/mgcpacer.go index ac3446db36..2d9fd27748 100644 --- 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 ", -- 2.48.1