cli,server: increase GOGC by default to 300%, add CLI flag

[nvanbenschoten]

Closes #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.

[cockroach-teamcity]

This change is 

[yuzefovich]

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?

[nvanbenschoten]

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

-- commits line 18 at r1:

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-memory nor GOMEMLIMIT).

Done.

---

pkg/cli/start.go line 561 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

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.

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.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?

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]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

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.

[nvanbenschoten]

TFTR!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)
• macos_amd64_build
• macos_arm64_build
• windows_build