Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: runtime: GC pacer redesign #44167

Open
mknyszek opened this issue Feb 8, 2021 · 26 comments
Open

proposal: runtime: GC pacer redesign #44167

mknyszek opened this issue Feb 8, 2021 · 26 comments
Assignees
Projects
Milestone

Comments

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Feb 8, 2021

GC Pacer Redesign

Author: Michael Knyszek (with lots of input from Austin Clements, David Chase, and Jeremy Faller)

Abstract

Go's tracing garbage collector runs concurrently with the application, and thus requires an algorithm to determine when to start a new cycle. In the runtime, this algorithm is referred to as the pacer. Until now, the garbage collector has framed this process as an optimization problem, utilizing a proportional controller to achieve a desired stopping-point (that is, the cycle completes just as the heap reaches a certain size) as well as a desired CPU utilization. While this approach has served Go well for a long time, the design has accrued many corner cases due to resolved issues, as well as a backlog of unresolved issues.

I propose redesigning the garbage collector's pacer from the ground up to capture the things it does well and eliminate the problems that have been discovered.

More specifically, I propose:

  1. Including all non-heap sources of GC work (stacks, globals) in pacing decisions.
  2. Reframing the pacing problem as a search problem, "solved" by a proportional-integral controller,
  3. Extending the hard heap goal to the worst-case heap goal of the next GC,

(1) will resolve long-standing issues with small heap sizes, allowing the Go garbage collector to scale down and act more predictably in general.
(2) will eliminate offset error present in the current design, will allow turning off mark-assist almost entirely outside of exceptional cases, improving allocation latency, and will enable clearer designs for setting memory limits on Go applications.
(3) will enable smooth and consistent response to large changes in the live heap size with large GOGC values.

Full design

Found here.

@mknyszek mknyszek added this to the Go1.17 milestone Feb 8, 2021
@mknyszek mknyszek self-assigned this Feb 8, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 8, 2021

Change https://golang.org/cl/290489 mentions this issue: design: add GC pacer redesign

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Feb 8, 2021
@mknyszek
Copy link
Contributor Author

@mknyszek mknyszek commented Feb 8, 2021

By the way: this design feels solid to me, but has not gone through any rounds of feedback yet. In the interest of transparency, I'm hoping to get feedback and work on this here on GitHub going forward.

So, given that, I would not be surprised if there are errors in the document. Please take a look when you have a chance!

CC @randall77 @jeremyfaller @dr2chase @aclements

@storozhukBM
Copy link

@storozhukBM storozhukBM commented Feb 12, 2021

@mknyszek

Do I understand correctly that the forcegcperiod is required because the current pacer does not consider non-heap sources of GC work? Is it necessary to call GC periodically in application with effectively zero heap allocation rate to collects stacks, etc.? If I understood your proposal correctly, it seems like it should be possible to remove these periodic calls of GC, and applications that don't create new goroutines and don't allocate anything on heap should never trigger garbage collections, which is a good benefit by itself.

@mknyszek
Copy link
Contributor Author

@mknyszek mknyszek commented Feb 13, 2021

@storozhukBM forcegcperiod is pretty much completely separate from this proposal. Not accounting for non-heap sources of work more directly affects the minimum heap size and whether the GC reaches its goals. Related though is #44163, which is partly motivated by some of the ill effects of the forcegcperiod trigger.

I believe forcegcperiod exists these days to help finalizers run in long-running mostly-idle programs. For example, if you have an external resource tied to the finalizer, you want that cleaned up eventually (and sooner, probably). Going by the API it need not ever run. But, then we could just turn the forced GC on only if there's active finalizers or something, so I'm not convinced that's the only reason. Shrinking stacks periodically also might be a reason, but that seems less critical.

Anyway, I have to look into this again so don't quote me. My memory is hazy. :) I'll dig into the reasons why next week (I don't see them documented anywhere).

gopherbot pushed a commit to golang/proposal that referenced this issue Feb 16, 2021
For golang/go#44167.

Change-Id: I468aa78edb8588b4e48008ad44cecc08544a8f48
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/290489
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Jeremy Faller <jeremy@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 16, 2021

Change https://golang.org/cl/292789 mentions this issue: design: add user-configurable memory target

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 18, 2021

Change https://golang.org/cl/293790 mentions this issue: design: regenerate graphs for GC pacer redesign

gopherbot pushed a commit to golang/proposal that referenced this issue Feb 19, 2021
A couple of the graphs were wrong (from the wrong scenario, that is)
because I copied them in manually. Fatal mistake.

Regenerate the graphs following the usual pipeline. Because there's
a degree of jitter and randomness in these graphs they end up slightly
different, but they're all mostly the same.

By regenerating these graphs, it also adds a new line to each graph
for the live heap size. I think this is nice for readability, so I'll
let that get updated too.

For golang/go#44167.

Change-Id: I097f812ba07ca7fd740d8460e2830de6492b3945
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/293790
Reviewed-by: Michael Pratt <mpratt@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 23, 2021

Change https://golang.org/cl/295509 mentions this issue: design: add initial conditions section to GC pacer redesign

gopherbot pushed a commit to golang/proposal that referenced this issue Feb 23, 2021
I realized I neglected to talk about initial conditions, even though all
the simulations clearly set *something*.

For golang/go#44167.

Change-Id: Ia1727d5c068847e9192bf87bc1b6a5f0bb832303
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/295509
Reviewed-by: Michael Pratt <mpratt@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306605 mentions this issue: runtime: make gcEffectiveGrowthRatio a method on gcControllerState

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306603 mentions this issue: runtime: move next_gc and last_next_gc into gcControllerState

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306599 mentions this issue: runtime: make gcSetTriggerRatio a method of gcControllerState

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306600 mentions this issue: runtime: move gcPercent and heapMinimum into gcControllerState

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306596 mentions this issue: runtime: break out GC pacer into its own file

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306597 mentions this issue: runtime: rename gcpercent, readgogc, and heapminimum to match Go style

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306602 mentions this issue: runtime: create setGCPercent method for gcControllerState

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306604 mentions this issue: runtime: pass work.userForced to gcController.endCycle explicitly

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306601 mentions this issue: runtime: create initializer for gcControllerState

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2021

Change https://golang.org/cl/306598 mentions this issue: runtime: move internal GC statistics from memstats to gcController

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2021

Change https://golang.org/cl/308690 mentions this issue: runtime: formalize and fix gcPercent synchronization

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 11, 2021

Change https://golang.org/cl/309274 mentions this issue: runtime: move pacer time updates and state resets into methods

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 11, 2021

Change https://golang.org/cl/309273 mentions this issue: runtime: detangle sweeper pacing from GC pacing

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 11, 2021

Change https://golang.org/cl/309275 mentions this issue: runtime: move heapLive and heapScan updates into a method

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 12, 2021

Change https://golang.org/cl/309590 mentions this issue: runtime: track scannable globals space

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 12, 2021

Change https://golang.org/cl/309589 mentions this issue: runtime: track the amount of allocated stack for the GC pacer

gopherbot pushed a commit that referenced this issue Apr 13, 2021
This change breaks out the GC pacer into its own file so that it'll be
easier to see the full implementation and change it. It also suggests an
obvious place to put tests (mgcpacer_test.go).

This includes all of gcControllerState, gcSetTriggerRatio, anything
related to GOGC, and all related globals and constants.

This is almost a clean move, except that globals and constants are
formatted into blocks instead of having a separate "var" declaration for
each one.

For #44167.

Change-Id: I85aa84ce85c6cfbe0b33e8a3c91cbe9dc41de8cb
Reviewed-on: https://go-review.googlesource.com/c/go/+/306596
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Apr 13, 2021
Generated with:

rf 'mv gcpercent gcPercent'
rf 'mv readgogc readGOGC'
rf 'mv heapminimum heapMinimum'

After this, comments referencing these symbols were updated via a simple
sed command.

For #44167.

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

@gopherbot gopherbot commented Apr 13, 2021

Change https://golang.org/cl/309869 mentions this issue: runtime: implement GC pacer redesign

gopherbot pushed a commit that referenced this issue Apr 13, 2021
This change moves certain important but internal-only GC statistics from
memstats into gcController. These statistics are mainly used in pacing
the GC, so it makes sense to keep them in the pacer's state.

This CL was mostly generated via

rf '
    ex . {
	memstats.gc_trigger -> gcController.trigger
	memstats.triggerRatio -> gcController.triggerRatio
	memstats.heap_marked -> gcController.heapMarked
	memstats.heap_live -> gcController.heapLive
	memstats.heap_scan -> gcController.heapScan
    }
'

except for a few special cases, like updating names in comments and when
these fields are used within gcControllerState methods (at which point
they're accessed through the reciever).

For #44167.

Change-Id: I6bd1602585aeeb80818ded24c07d8e6fec992b93
Reviewed-on: https://go-review.googlesource.com/c/go/+/306598
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Apr 14, 2021
gcSetTriggerRatio's purpose is to set a bunch of downstream values when
we choose to commit to a new trigger ratio computed by the gcController.
Now that almost all the inputs it uses to compute the downstream values
are in gcControllerState anyway, make it a method of gcControllerState.

For #44167.

Change-Id: I1b7ea709e8378566f812ae3450ab169d7fb66aea
Reviewed-on: https://go-review.googlesource.com/c/go/+/306599
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Apr 14, 2021
These variables are core to the pacer, and will be need to be non-global
for testing later.

Partially generated via

rf '
    ex . {
	gcPercent -> gcController.gcPercent
	heapMinimum -> gcController.heapMinimum
    }
'

The only exception to this generation is usage of these variables
in gcControllerState methods.

For #44167.

Change-Id: I8b620b3061114f3a3c4b65006f715fd977b180a8
Reviewed-on: https://go-review.googlesource.com/c/go/+/306600
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Apr 14, 2021
Now that gcControllerState contains almost all of the pacer state,
create an initializer for it instead of haphazardly setting some fields.

For #44167.

Change-Id: I4ce1d5dd82003cb7c263fa46697851bb22a32544
Reviewed-on: https://go-review.googlesource.com/c/go/+/306601
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Apr 14, 2021
This change breaks out the computations done by setGCPercent into
a method on gcControllerState for easier testing later. It leaves behind
the global implementation details.

For #44167.

Change-Id: I3b0cf1475b032fcd4ebbd01cf4e80de0b55ce7b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/306602
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Apr 14, 2021
This change moves next_gc and last_next_gc into gcControllerState under
the names heapGoal and lastHeapGoal respectively. These are
fundamentally GC pacer related values, and so it makes sense for them to
live here.

Partially generated by

rf '
    ex . {
	memstats.next_gc -> gcController.heapGoal
	memstats.last_next_gc -> gcController.lastHeapGoal
    }
'

except for updates to comments and gcControllerState methods, where
they're accessed through the receiver, and trace-related renames of
NextGC -> HeapGoal, while we're here.

For #44167.

Change-Id: I1e871ad78a57b01be8d9f71bd662530c84853bed
Reviewed-on: https://go-review.googlesource.com/c/go/+/306603
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Apr 14, 2021
For #44167.

Change-Id: I15817006f1870d6237cd06dabad988da3f23a6d6
Reviewed-on: https://go-review.googlesource.com/c/go/+/306604
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Apr 14, 2021
For #44167.

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

@mknyszek mknyszek commented Apr 15, 2021

Unfortunately with everything already going into this release, I need to push this back.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 16, 2021

Change https://golang.org/cl/350429 mentions this issue: design: update gc-pacer-redesign and remove inaccuracies

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants