]> Cypherpunks.ru repositories - gostls13.git/blobdiff - src/runtime/mgcpacer.go
runtime: refactor runtime->tracer API to appear more like a lock
[gostls13.git] / src / runtime / mgcpacer.go
index b8483cc12b563f110580b3f036b19f23d45d4476..716e3efcccebba33414183211a6784a3bb2c5ef9 100644 (file)
@@ -11,12 +11,6 @@ import (
        _ "unsafe" // for go:linkname
 )
 
-// go119MemoryLimitSupport is a feature flag for a number of changes
-// related to the memory limit feature (#48409). Disabling this flag
-// disables those features, as well as the memory limit mechanism,
-// which becomes a no-op.
-const go119MemoryLimitSupport = true
-
 const (
        // gcGoalUtilization is the goal CPU utilization for
        // marking as a fraction of GOMAXPROCS.
@@ -67,11 +61,16 @@ const (
        // that can accumulate on a P before updating gcController.stackSize.
        maxStackScanSlack = 8 << 10
 
-       // memoryLimitHeapGoalHeadroom is the amount of headroom the pacer gives to
-       // the heap goal when operating in the memory-limited regime. That is,
-       // it'll reduce the heap goal by this many extra bytes off of the base
-       // calculation.
-       memoryLimitHeapGoalHeadroom = 1 << 20
+       // memoryLimitMinHeapGoalHeadroom is the minimum amount of headroom the
+       // pacer gives to the heap goal when operating in the memory-limited regime.
+       // That is, it'll reduce the heap goal by this many extra bytes off of the
+       // base calculation, at minimum.
+       memoryLimitMinHeapGoalHeadroom = 1 << 20
+
+       // memoryLimitHeapGoalHeadroomPercent is how headroom the memory-limit-based
+       // heap goal should have as a percent of the maximum possible heap goal allowed
+       // to maintain the memory limit.
+       memoryLimitHeapGoalHeadroomPercent = 3
 )
 
 // gcController implements the GC pacing controller that determines
@@ -92,8 +91,6 @@ type gcControllerState struct {
        // Initialized from GOGC. GOGC=off means no GC.
        gcPercent atomic.Int32
 
-       _ uint32 // padding so following 64-bit values are 8-byte aligned
-
        // memoryLimit is the soft memory limit in bytes.
        //
        // Initialized from GOMEMLIMIT. GOMEMLIMIT=off is equivalent to MaxInt64
@@ -138,14 +135,10 @@ type gcControllerState struct {
        // Updated at the end of each GC cycle, in endCycle.
        consMark float64
 
-       // consMarkController holds the state for the mark-cons ratio
-       // estimation over time.
-       //
-       // Its purpose is to smooth out noisiness in the computation of
-       // consMark; see consMark for details.
-       consMarkController piController
-
-       _ uint32 // Padding for atomics on 32-bit platforms.
+       // lastConsMark is the computed cons/mark value for the previous 4 GC
+       // cycles. Note that this is *not* the last value of consMark, but the
+       // measured cons/mark value in endCycle.
+       lastConsMark [4]float64
 
        // gcPercentHeapGoal is the goal heapLive for when next GC ends derived
        // from gcPercent.
@@ -270,31 +263,29 @@ type gcControllerState struct {
        // written and read throughout the cycle.
        assistTime atomic.Int64
 
-       // dedicatedMarkTime is the nanoseconds spent in dedicated
-       // mark workers during this cycle. This is updated atomically
-       // at the end of the concurrent mark phase.
-       dedicatedMarkTime int64
+       // dedicatedMarkTime is the nanoseconds spent in dedicated mark workers
+       // during this cycle. This is updated at the end of the concurrent mark
+       // phase.
+       dedicatedMarkTime atomic.Int64
 
-       // fractionalMarkTime is the nanoseconds spent in the
-       // fractional mark worker during this cycle. This is updated
-       // atomically throughout the cycle and will be up-to-date if
-       // the fractional mark worker is not currently running.
-       fractionalMarkTime int64
+       // fractionalMarkTime is the nanoseconds spent in the fractional mark
+       // worker during this cycle. This is updated throughout the cycle and
+       // will be up-to-date if the fractional mark worker is not currently
+       // running.
+       fractionalMarkTime atomic.Int64
 
-       // idleMarkTime is the nanoseconds spent in idle marking
-       // during this cycle. This is updated atomically throughout
-       // the cycle.
-       idleMarkTime int64
+       // idleMarkTime is the nanoseconds spent in idle marking during this
+       // cycle. This is updated throughout the cycle.
+       idleMarkTime atomic.Int64
 
        // markStartTime is the absolute start time in nanoseconds
        // that assists and background mark workers started.
        markStartTime int64
 
-       // dedicatedMarkWorkersNeeded is the number of dedicated mark
-       // workers that need to be started. This is computed at the
-       // beginning of each cycle and decremented atomically as
-       // dedicated mark workers get started.
-       dedicatedMarkWorkersNeeded int64
+       // dedicatedMarkWorkersNeeded is the number of dedicated mark workers
+       // that need to be started. This is computed at the beginning of each
+       // cycle and decremented as dedicated mark workers get started.
+       dedicatedMarkWorkersNeeded atomic.Int64
 
        // idleMarkWorkers is two packed int32 values in a single uint64.
        // These two values are always updated simultaneously.
@@ -378,28 +369,6 @@ type gcControllerState struct {
 func (c *gcControllerState) init(gcPercent int32, memoryLimit int64) {
        c.heapMinimum = defaultHeapMinimum
        c.triggered = ^uint64(0)
-
-       c.consMarkController = piController{
-               // Tuned first via the Ziegler-Nichols process in simulation,
-               // then the integral time was manually tuned against real-world
-               // applications to deal with noisiness in the measured cons/mark
-               // ratio.
-               kp: 0.9,
-               ti: 4.0,
-
-               // Set a high reset time in GC cycles.
-               // This is inversely proportional to the rate at which we
-               // accumulate error from clipping. By making this very high
-               // we make the accumulation slow. In general, clipping is
-               // OK in our situation, hence the choice.
-               //
-               // Tune this if we get unintended effects from clipping for
-               // a long time.
-               tt:  1000,
-               min: -1000,
-               max: 1000,
-       }
-
        c.setGCPercent(gcPercent)
        c.setMemoryLimit(memoryLimit)
        c.commit(true) // No sweep phase in the first GC cycle.
@@ -418,30 +387,11 @@ func (c *gcControllerState) startCycle(markStartTime int64, procs int, trigger g
        c.globalsScanWork.Store(0)
        c.bgScanCredit.Store(0)
        c.assistTime.Store(0)
-       c.dedicatedMarkTime = 0
-       c.fractionalMarkTime = 0
-       c.idleMarkTime = 0
+       c.dedicatedMarkTime.Store(0)
+       c.fractionalMarkTime.Store(0)
+       c.idleMarkTime.Store(0)
        c.markStartTime = markStartTime
-
-       // 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
+       c.triggered = c.heapLive.Load()
 
        // Compute the background mark utilization goal. In general,
        // this may not come out exactly. We round the number of
@@ -449,26 +399,26 @@ func (c *gcControllerState) startCycle(markStartTime int64, procs int, trigger g
        // 25%. For small GOMAXPROCS, this would introduce too much
        // error, so we add fractional workers in that case.
        totalUtilizationGoal := float64(procs) * gcBackgroundUtilization
-       c.dedicatedMarkWorkersNeeded = int64(totalUtilizationGoal + 0.5)
-       utilError := float64(c.dedicatedMarkWorkersNeeded)/totalUtilizationGoal - 1
+       dedicatedMarkWorkersNeeded := int64(totalUtilizationGoal + 0.5)
+       utilError := float64(dedicatedMarkWorkersNeeded)/totalUtilizationGoal - 1
        const maxUtilError = 0.3
        if utilError < -maxUtilError || utilError > maxUtilError {
                // Rounding put us more than 30% off our goal. With
                // gcBackgroundUtilization of 25%, this happens for
                // GOMAXPROCS<=3 or GOMAXPROCS=6. Enable fractional
                // workers to compensate.
-               if float64(c.dedicatedMarkWorkersNeeded) > totalUtilizationGoal {
+               if float64(dedicatedMarkWorkersNeeded) > totalUtilizationGoal {
                        // Too many dedicated workers.
-                       c.dedicatedMarkWorkersNeeded--
+                       dedicatedMarkWorkersNeeded--
                }
-               c.fractionalUtilizationGoal = (totalUtilizationGoal - float64(c.dedicatedMarkWorkersNeeded)) / float64(procs)
+               c.fractionalUtilizationGoal = (totalUtilizationGoal - float64(dedicatedMarkWorkersNeeded)) / float64(procs)
        } else {
                c.fractionalUtilizationGoal = 0
        }
 
        // In STW mode, we just want dedicated workers.
        if debug.gcstoptheworld > 0 {
-               c.dedicatedMarkWorkersNeeded = int64(procs)
+               dedicatedMarkWorkersNeeded = int64(procs)
                c.fractionalUtilizationGoal = 0
        }
 
@@ -483,7 +433,7 @@ func (c *gcControllerState) startCycle(markStartTime int64, procs int, trigger g
                // required. However, we need at least one dedicated mark worker or
                // idle GC worker to ensure GC progress in some scenarios (see comment
                // on maxIdleMarkWorkers).
-               if c.dedicatedMarkWorkersNeeded > 0 {
+               if dedicatedMarkWorkersNeeded > 0 {
                        c.setMaxIdleMarkWorkers(0)
                } else {
                        // TODO(mknyszek): The fundamental reason why we need this is because
@@ -493,22 +443,24 @@ func (c *gcControllerState) startCycle(markStartTime int64, procs int, trigger g
                        c.setMaxIdleMarkWorkers(1)
                }
        } else {
-               // N.B. gomaxprocs and dedicatedMarkWorkersNeeded is guaranteed not to
+               // N.B. gomaxprocs and dedicatedMarkWorkersNeeded are guaranteed not to
                // change during a GC cycle.
-               c.setMaxIdleMarkWorkers(int32(procs) - int32(c.dedicatedMarkWorkersNeeded))
+               c.setMaxIdleMarkWorkers(int32(procs) - int32(dedicatedMarkWorkersNeeded))
        }
 
        // Compute initial values for controls that are updated
        // throughout the cycle.
+       c.dedicatedMarkWorkersNeeded.Store(dedicatedMarkWorkersNeeded)
        c.revise()
 
        if debug.gcpacertrace > 0 {
+               heapGoal := c.heapGoal()
                assistRatio := c.assistWorkPerByte.Load()
                print("pacer: assist ratio=", assistRatio,
                        " (scan ", gcController.heapScan.Load()>>20, " MB in ",
                        work.initialHeapLive>>20, "->",
                        heapGoal>>20, " MB)",
-                       " workers=", c.dedicatedMarkWorkersNeeded,
+                       " workers=", dedicatedMarkWorkersNeeded,
                        "+", c.fractionalUtilizationGoal, "\n")
        }
 }
@@ -671,7 +623,7 @@ func (c *gcControllerState) endCycle(now int64, procs int, userForced bool) {
        }
        idleUtilization := 0.0
        if assistDuration > 0 {
-               idleUtilization = float64(c.idleMarkTime) / float64(assistDuration*int64(procs))
+               idleUtilization = float64(c.idleMarkTime.Load()) / float64(assistDuration*int64(procs))
        }
        // Determine the cons/mark ratio.
        //
@@ -689,7 +641,7 @@ func (c *gcControllerState) endCycle(now int64, procs int, userForced bool) {
        //
        //    assistDuration * procs * (utilization + idleUtilization)
        //
-       // In this case, we *include* idle utilization, because that is additional CPU time that the
+       // In this case, we *include* idle utilization, because that is additional CPU time that
        // the GC had available to it.
        //
        // In effect, idle GC time is sort of double-counted here, but it's very weird compared
@@ -698,38 +650,26 @@ func (c *gcControllerState) endCycle(now int64, procs int, userForced bool) {
        //
        // So this calculation is really:
        //     (heapLive-trigger) / (assistDuration * procs * (1-utilization)) /
-       //         (scanWork) / (assistDuration * procs * (utilization+idleUtilization)
+       //         (scanWork) / (assistDuration * procs * (utilization+idleUtilization))
        //
        // Note that because we only care about the ratio, assistDuration and procs cancel out.
        scanWork := c.heapScanWork.Load() + c.stackScanWork.Load() + c.globalsScanWork.Load()
        currentConsMark := (float64(c.heapLive.Load()-c.triggered) * (utilization + idleUtilization)) /
                (float64(scanWork) * (1 - utilization))
 
-       // Update cons/mark controller. The time period for this is 1 GC cycle.
-       //
-       // This use of a PI controller might seem strange. So, here's an explanation:
-       //
-       // currentConsMark represents the consMark we *should've* had to be perfectly
-       // on-target for this cycle. Given that we assume the next GC will be like this
-       // one in the steady-state, it stands to reason that we should just pick that
-       // as our next consMark. In practice, however, currentConsMark is too noisy:
-       // we're going to be wildly off-target in each GC cycle if we do that.
-       //
-       // What we do instead is make a long-term assumption: there is some steady-state
-       // consMark value, but it's obscured by noise. By constantly shooting for this
-       // noisy-but-perfect consMark value, the controller will bounce around a bit,
-       // but its average behavior, in aggregate, should be less noisy and closer to
-       // the true long-term consMark value, provided its tuned to be slightly overdamped.
-       var ok bool
+       // Update our cons/mark estimate. This is the maximum of the value we just computed and the last
+       // 4 cons/mark values we measured. The reason we take the maximum here is to bias a noisy
+       // cons/mark measurement toward fewer assists at the expense of additional GC cycles (starting
+       // earlier).
        oldConsMark := c.consMark
-       c.consMark, ok = c.consMarkController.next(c.consMark, currentConsMark, 1.0)
-       if !ok {
-               // The error spiraled out of control. This is incredibly unlikely seeing
-               // as this controller is essentially just a smoothing function, but it might
-               // mean that something went very wrong with how currentConsMark was calculated.
-               // Just reset consMark and keep going.
-               c.consMark = 0
+       c.consMark = currentConsMark
+       for i := range c.lastConsMark {
+               if c.lastConsMark[i] > c.consMark {
+                       c.consMark = c.lastConsMark[i]
+               }
        }
+       copy(c.lastConsMark[:], c.lastConsMark[1:])
+       c.lastConsMark[len(c.lastConsMark)-1] = currentConsMark
 
        if debug.gcpacertrace > 0 {
                printlock()
@@ -738,9 +678,6 @@ func (c *gcControllerState) endCycle(now int64, procs int, userForced bool) {
                print(c.heapScanWork.Load(), "+", c.stackScanWork.Load(), "+", c.globalsScanWork.Load(), " B work (", c.lastHeapScan+c.lastStackScan.Load()+c.globalsScan.Load(), " B exp.) ")
                live := c.heapLive.Load()
                print("in ", c.triggered, " B -> ", live, " B (∆goal ", int64(live)-int64(c.lastHeapGoal), ", cons/mark ", oldConsMark, ")")
-               if !ok {
-                       print("[controller reset]")
-               }
                println()
                printunlock()
        }
@@ -755,14 +692,14 @@ func (c *gcControllerState) enlistWorker() {
        // If there are idle Ps, wake one so it will run an idle worker.
        // NOTE: This is suspected of causing deadlocks. See golang.org/issue/19112.
        //
-       //      if atomic.Load(&sched.npidle) != 0 && atomic.Load(&sched.nmspinning) == 0 {
+       //      if sched.npidle.Load() != 0 && sched.nmspinning.Load() == 0 {
        //              wakep()
        //              return
        //      }
 
        // There are no idle Ps. If we need more dedicated workers,
        // try to preempt a running P so it will switch to a worker.
-       if c.dedicatedMarkWorkersNeeded <= 0 {
+       if c.dedicatedMarkWorkersNeeded.Load() <= 0 {
                return
        }
        // Pick a random other P to preempt.
@@ -832,14 +769,14 @@ func (c *gcControllerState) findRunnableGCWorker(pp *p, now int64) (*g, int64) {
                return nil, now
        }
 
-       decIfPositive := func(ptr *int64) bool {
+       decIfPositive := func(val *atomic.Int64) bool {
                for {
-                       v := atomic.Loadint64(ptr)
+                       v := val.Load()
                        if v <= 0 {
                                return false
                        }
 
-                       if atomic.Casint64(ptr, v, v-1) {
+                       if val.CompareAndSwap(v, v-1) {
                                return true
                        }
                }
@@ -870,9 +807,11 @@ func (c *gcControllerState) findRunnableGCWorker(pp *p, now int64) (*g, int64) {
 
        // Run the background mark worker.
        gp := node.gp.ptr()
+       trace := traceAcquire()
        casgstatus(gp, _Gwaiting, _Grunnable)
-       if trace.enabled {
-               traceGoUnpark(gp, 0)
+       if trace.ok() {
+               trace.GoUnpark(gp, 0)
+               traceRelease(trace)
        }
        return gp, now
 }
@@ -891,8 +830,10 @@ func (c *gcControllerState) resetLive(bytesMarked uint64) {
        c.triggered = ^uint64(0) // Reset triggered.
 
        // heapLive was updated, so emit a trace event.
-       if trace.enabled {
-               traceHeapAlloc(bytesMarked)
+       trace := traceAcquire()
+       if trace.ok() {
+               trace.HeapAlloc(bytesMarked)
+               traceRelease(trace)
        }
 }
 
@@ -905,12 +846,12 @@ func (c *gcControllerState) resetLive(bytesMarked uint64) {
 func (c *gcControllerState) markWorkerStop(mode gcMarkWorkerMode, duration int64) {
        switch mode {
        case gcMarkWorkerDedicatedMode:
-               atomic.Xaddint64(&c.dedicatedMarkTime, duration)
-               atomic.Xaddint64(&c.dedicatedMarkWorkersNeeded, 1)
+               c.dedicatedMarkTime.Add(duration)
+               c.dedicatedMarkWorkersNeeded.Add(1)
        case gcMarkWorkerFractionalMode:
-               atomic.Xaddint64(&c.fractionalMarkTime, duration)
+               c.fractionalMarkTime.Add(duration)
        case gcMarkWorkerIdleMode:
-               atomic.Xaddint64(&c.idleMarkTime, duration)
+               c.idleMarkTime.Add(duration)
                c.removeIdleMarkWorker()
        default:
                throw("markWorkerStop: unknown mark worker mode")
@@ -919,10 +860,12 @@ func (c *gcControllerState) markWorkerStop(mode gcMarkWorkerMode, duration int64
 
 func (c *gcControllerState) update(dHeapLive, dHeapScan int64) {
        if dHeapLive != 0 {
+               trace := traceAcquire()
                live := gcController.heapLive.Add(dHeapLive)
-               if trace.enabled {
+               if trace.ok() {
                        // gcController.heapLive changed.
-                       traceHeapAlloc(live)
+                       trace.HeapAlloc(live)
+                       traceRelease(trace)
                }
        }
        if gcBlackenEnabled == 0 {
@@ -968,7 +911,7 @@ func (c *gcControllerState) heapGoalInternal() (goal, minTrigger uint64) {
        goal = c.gcPercentHeapGoal.Load()
 
        // Check if the memory-limit-based goal is smaller, and if so, pick that.
-       if newGoal := c.memoryLimitHeapGoal(); go119MemoryLimitSupport && newGoal < goal {
+       if newGoal := c.memoryLimitHeapGoal(); newGoal < goal {
                goal = newGoal
        } else {
                // We're not limited by the memory limit goal, so perform a series of
@@ -1036,8 +979,10 @@ func (c *gcControllerState) memoryLimitHeapGoal() uint64 {
        //
        // In practice this computation looks like the following:
        //
-       //    memoryLimit - ((mappedReady - heapFree - heapAlloc) + max(mappedReady - memoryLimit, 0)) - memoryLimitHeapGoalHeadroom
-       //                    ^1                                    ^2                                   ^3
+       //    goal := memoryLimit - ((mappedReady - heapFree - heapAlloc) + max(mappedReady - memoryLimit, 0))
+       //                    ^1                                    ^2
+       //    goal -= goal / 100 * memoryLimitHeapGoalHeadroomPercent
+       //    ^3
        //
        // Let's break this down.
        //
@@ -1069,11 +1014,14 @@ func (c *gcControllerState) memoryLimitHeapGoal() uint64 {
        // terms of heap objects, but it takes more than X bytes (e.g. due to fragmentation) to store
        // X bytes worth of objects.
        //
-       // The third term (marker 3) subtracts an additional memoryLimitHeapGoalHeadroom bytes from the
-       // heap goal. As the name implies, this is to provide additional headroom in the face of pacing
-       // inaccuracies. This is a fixed number of bytes because these inaccuracies disproportionately
-       // affect small heaps: as heaps get smaller, the pacer's inputs get fuzzier. Shorter GC cycles
-       // and less GC work means noisy external factors like the OS scheduler have a greater impact.
+       // The final adjustment (marker 3) reduces the maximum possible memory limit heap goal by
+       // memoryLimitHeapGoalPercent. As the name implies, this is to provide additional headroom in
+       // the face of pacing inaccuracies, and also to leave a buffer of unscavenged memory so the
+       // allocator isn't constantly scavenging. The reduction amount also has a fixed minimum
+       // (memoryLimitMinHeapGoalHeadroom, not pictured) because the aforementioned pacing inaccuracies
+       // disproportionately affect small heaps: as heaps get smaller, the pacer's inputs get fuzzier.
+       // Shorter GC cycles and less GC work means noisy external factors like the OS scheduler have a
+       // greater impact.
 
        memoryLimit := uint64(c.memoryLimit.Load())
 
@@ -1097,12 +1045,19 @@ func (c *gcControllerState) memoryLimitHeapGoal() uint64 {
        // Compute the goal.
        goal := memoryLimit - (nonHeapMemory + overage)
 
-       // Apply some headroom to the goal to account for pacing inaccuracies.
-       // Be careful about small limits.
-       if goal < memoryLimitHeapGoalHeadroom || goal-memoryLimitHeapGoalHeadroom < memoryLimitHeapGoalHeadroom {
-               goal = memoryLimitHeapGoalHeadroom
+       // Apply some headroom to the goal to account for pacing inaccuracies and to reduce
+       // the impact of scavenging at allocation time in response to a high allocation rate
+       // when GOGC=off. See issue #57069. Also, be careful about small limits.
+       headroom := goal / 100 * memoryLimitHeapGoalHeadroomPercent
+       if headroom < memoryLimitMinHeapGoalHeadroom {
+               // Set a fixed minimum to deal with the particularly large effect pacing inaccuracies
+               // have for smaller heaps.
+               headroom = memoryLimitMinHeapGoalHeadroom
+       }
+       if goal < headroom || goal-headroom < headroom {
+               goal = headroom
        } else {
-               goal = goal - memoryLimitHeapGoalHeadroom
+               goal = goal - headroom
        }
        // Don't let us go below the live heap. A heap goal below the live heap doesn't make sense.
        if goal < c.heapMarked {
@@ -1170,7 +1125,7 @@ func (c *gcControllerState) trigger() (uint64, uint64) {
        // increase in RSS. By capping us at a point >0, we're essentially
        // saying that we're OK using more CPU during the GC to prevent
        // this growth in RSS.
-       triggerLowerBound := uint64(((goal-c.heapMarked)/triggerRatioDen)*minTriggerRatioNum) + c.heapMarked
+       triggerLowerBound := ((goal-c.heapMarked)/triggerRatioDen)*minTriggerRatioNum + c.heapMarked
        if minTrigger < triggerLowerBound {
                minTrigger = triggerLowerBound
        }
@@ -1184,13 +1139,11 @@ func (c *gcControllerState) trigger() (uint64, uint64) {
        // to reflect the costs of a GC with no work to do. With a large heap but
        // very little scan work to perform, this gives us exactly as much runway
        // as we would need, in the worst case.
-       maxTrigger := uint64(((goal-c.heapMarked)/triggerRatioDen)*maxTriggerRatioNum) + c.heapMarked
+       maxTrigger := ((goal-c.heapMarked)/triggerRatioDen)*maxTriggerRatioNum + c.heapMarked
        if goal > defaultHeapMinimum && goal-defaultHeapMinimum > maxTrigger {
                maxTrigger = goal - defaultHeapMinimum
        }
-       if maxTrigger < minTrigger {
-               maxTrigger = minTrigger
-       }
+       maxTrigger = max(maxTrigger, minTrigger)
 
        // Compute the trigger from our bounds and the runway stored by commit.
        var trigger uint64
@@ -1200,12 +1153,8 @@ func (c *gcControllerState) trigger() (uint64, uint64) {
        } else {
                trigger = goal - runway
        }
-       if trigger < minTrigger {
-               trigger = minTrigger
-       }
-       if trigger > maxTrigger {
-               trigger = maxTrigger
-       }
+       trigger = max(trigger, minTrigger)
+       trigger = min(trigger, maxTrigger)
        if trigger > goal {
                print("trigger=", trigger, " heapGoal=", goal, "\n")
                print("minTrigger=", minTrigger, " maxTrigger=", maxTrigger, "\n")
@@ -1319,7 +1268,7 @@ func setGCPercent(in int32) (out int32) {
        // If we just disabled GC, wait for any concurrent GC mark to
        // finish so we always return with no GC running.
        if in < 0 {
-               gcWaitOnMark(atomic.Load(&work.cycles))
+               gcWaitOnMark(work.cycles.Load())
        }
 
        return out
@@ -1384,74 +1333,6 @@ func readGOMEMLIMIT() int64 {
        return n
 }
 
-type piController struct {
-       kp float64 // Proportional constant.
-       ti float64 // Integral time constant.
-       tt float64 // Reset time.
-
-       min, max float64 // Output boundaries.
-
-       // PI controller state.
-
-       errIntegral float64 // Integral of the error from t=0 to now.
-
-       // Error flags.
-       errOverflow   bool // Set if errIntegral ever overflowed.
-       inputOverflow bool // Set if an operation with the input overflowed.
-}
-
-// next provides a new sample to the controller.
-//
-// input is the sample, setpoint is the desired point, and period is how much
-// time (in whatever unit makes the most sense) has passed since the last sample.
-//
-// Returns a new value for the variable it's controlling, and whether the operation
-// completed successfully. One reason this might fail is if error has been growing
-// in an unbounded manner, to the point of overflow.
-//
-// In the specific case of an error overflow occurs, the errOverflow field will be
-// set and the rest of the controller's internal state will be fully reset.
-func (c *piController) next(input, setpoint, period float64) (float64, bool) {
-       // Compute the raw output value.
-       prop := c.kp * (setpoint - input)
-       rawOutput := prop + c.errIntegral
-
-       // Clamp rawOutput into output.
-       output := rawOutput
-       if isInf(output) || isNaN(output) {
-               // The input had a large enough magnitude that either it was already
-               // overflowed, or some operation with it overflowed.
-               // Set a flag and reset. That's the safest thing to do.
-               c.reset()
-               c.inputOverflow = true
-               return c.min, false
-       }
-       if output < c.min {
-               output = c.min
-       } else if output > c.max {
-               output = c.max
-       }
-
-       // Update the controller's state.
-       if c.ti != 0 && c.tt != 0 {
-               c.errIntegral += (c.kp*period/c.ti)*(setpoint-input) + (period/c.tt)*(output-rawOutput)
-               if isInf(c.errIntegral) || isNaN(c.errIntegral) {
-                       // So much error has accumulated that we managed to overflow.
-                       // The assumptions around the controller have likely broken down.
-                       // Set a flag and reset. That's the safest thing to do.
-                       c.reset()
-                       c.errOverflow = true
-                       return c.min, false
-               }
-       }
-       return output, true
-}
-
-// reset resets the controller state, except for controller error flags.
-func (c *piController) reset() {
-       c.errIntegral = 0
-}
-
 // addIdleMarkWorker attempts to add a new idle mark worker.
 //
 // If this returns true, the caller must become an idle mark worker unless
@@ -1553,8 +1434,10 @@ func gcControllerCommit() {
 
        // TODO(mknyszek): This isn't really accurate any longer because the heap
        // goal is computed dynamically. Still useful to snapshot, but not as useful.
-       if trace.enabled {
-               traceHeapGoal()
+       trace := traceAcquire()
+       if trace.ok() {
+               trace.HeapGoal()
+               traceRelease(trace)
        }
 
        trigger, heapGoal := gcController.trigger()