cli,server: increase GOGC by default to 300%, add CLI flag#119605
cli,server: increase GOGC by default to 300%, add CLI flag#119605craig[bot] merged 1 commit intocockroachdb:masterfrom
GOGC by default to 300%, add CLI flag#119605Conversation
|
This change is |
yuzefovich
left a comment
There was a problem hiding this comment.
Nice! I only have a few minor nits.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
-- commits line 18 at r1:
nit: perhaps explicitly mention that Go soft memory limit needs to be set (i.e. not explicitly disabled via --max-go-memory nor GOMEMLIMIT).
pkg/cli/start.go line 561 at r1 (raw file):
if err := func() error { var goGCPercent int if startCtx.goGCPercent != 0 {
It seems like currently --go-gc-percent=0 is effectively ignored. Should we perhaps, instead, treat --go-gc-percent=0 similar to GOGC=off? If so, we'll need to distinguish between this case and --go-gc-percent not being specified.
pkg/cli/start.go line 567 at r1 (raw file):
goGCPercent = startCtx.goGCPercent } else { if _, envVarSet := envutil.ExternalEnvString("GOGC", 1); envVarSet {
nit: would be nice to add a test similar to the one from #101498.
pkg/cli/start.go line 577 at r1 (raw file):
// also configured, to avoid introducing OOMs. goMemLimit := debug.SetMemoryLimit(-1 /* get without adjusting */) if goMemLimit == math.MaxInt64 {
nit: the contract of debug.SetMemoryLimit is such that any large number can effectively disable it. Thus, IIUC, it's valid to start the process with something like GOMEMLIMIT='1000GiB' which effectively disables the soft memory limit, but in this code we wouldn't treat it as such. Do you think it'd be worth it to change this condition to something like if goMemLimit > 1_000_000_000_000?
8473c09 to
b144c70
Compare
nvb
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: perhaps explicitly mention that Go soft memory limit needs to be set (i.e. not explicitly disabled via
--max-go-memorynorGOMEMLIMIT).
Done.
pkg/cli/start.go line 561 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
It seems like currently
--go-gc-percent=0is effectively ignored. Should we perhaps, instead, treat--go-gc-percent=0similar toGOGC=off? If so, we'll need to distinguish between this case and--go-gc-percentnot being specified.
Great point, done. I also mentioned in the documentation that the flag can be set to a negative value to turn off the GC percent heuristic, just like debug.SetGCPercent.
pkg/cli/start.go line 567 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: would be nice to add a test similar to the one from #101498.
I'm not sure what that is testing. Just that the process doesn't crash when a GOGC value is supplied? Regardless, done.
pkg/cli/start.go line 577 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: the contract of
debug.SetMemoryLimitis such that any large number can effectively disable it. Thus, IIUC, it's valid to start the process with something likeGOMEMLIMIT='1000GiB'which effectively disables the soft memory limit, but in this code we wouldn't treat it as such. Do you think it'd be worth it to change this condition to something likeif goMemLimit > 1_000_000_000_000?
I don't think it's worth doing that and introducing a hidden behavioral change threshold. If someone sets a very large GOMEMLIMIT then they're actively doing so for a reason. If this causes OOMs because the target percentage garbage collection heuristic isn't aggressive enough to enforce their true memory limit then they can either fix their GOMEMLIMIT or set GOGC or --go-gc-percent.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status:
pkg/cli/start.go line 567 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I'm not sure what that is testing. Just that the process doesn't crash when a GOGC value is supplied? Regardless, done.
Yeah, just that - before that fix, I used os.LookupEnv which led to a crash whenever GOMEMLIMIT env var was set.
Closes cockroachdb#115164. This commit introduces a new `--go-gc-percent` flag to CLI `start` command which controls the garbage collection target percentage on the Go runtime. The default value for the flag if neither the --go-gc-percent flag nor the GOGC env var is set is 300%. To avoid introducing new OOMs, we only switch to this new default (up from 100%) if a soft memory limit is set, either manually or automatically. Release note (cli change): A new flag `--go-gc-percent` is introduced to `start` command. It controls the garbage collection target percentage on the Go runtime, mirroring the existing GOGC environment variable. A garbage collection is triggered when the ratio of freshly allocated data to live data remaining after the previous collection reaches this percentage. If left unspecified and a Go soft memory limit is configured (i.e. not explicitly disabled via --max-go-memory nor GOMEMLIMIT), the GC target percentage defaults to 300%. If set to a negative value, disables the target percentage garbage collection heuristic, leaving only the soft memory limit heuristic to trigger garbage collection.
b144c70 to
d859f6b
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
Closes #115164.
This commit introduces a new
--go-gc-percentflag to CLIstartcommand which controls the garbage collection target percentage on the Go runtime. The default value for the flag if neither the --go-gc-percent flag nor the GOGC env var is set is 300%.To avoid introducing new OOMs, we only switch to this new default (up from 100%) if a soft memory limit is set, either manually or automatically.
Release note (cli change): A new flag
--go-gc-percentis introduced tostartcommand. It controls the garbage collection target percentage on the Go runtime, mirroring the existing GOGC environment variable. A garbage collection is triggered when the ratio of freshly allocated data to live data remaining after the previous collection reaches this percentage. If left unspecified and a Go soft memory limit is configured (i.e. not explicitly disabled via --max-go-memory nor GOMEMLIMIT), the GC target percentage defaults to 300%. If set to a negative value, disables the target percentage garbage collection heuristic, leaving only the soft memory limit heuristic to trigger garbage collection.