cockroachdb / cockroachdb/cockroach
admission: root cause & fix the slight negative perf impact of cpuTimeTokenFiller
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
An initial version of https://github.com/cockroachdb/cockroach/pull/161179 had a small but but statistically significant regression in SQL level benchmark results. That PR hooked in CPU time token AC, but slot-based AC is still enabled by default, so CPU time token AC is not in the hot path. The likely proximate cause was thus the `cpuTimeTokenFiller` goroutine, which runs every 1ms. Some benchmarking was done of `cpuTimeTokenFiller`, and no glaring issues were found. Still, making the filler background process start only when CPU time token AC was enabled resolved the regression:
```
// The filler ticking appears to have a slight negative impact on perf.
// For now, we accept this, since CPU time token AC will be off by
// default, and only enabled in Serverless. To track fixing the perf
// issue, we have the following ticket:
// https://github.com/cockroachdb/cockroach/issues/161945
if !knobs.DisableCPUTimeTokenFillerGoroutine {
var fillerStarted bool
cpuTimeTokenACEnabled.SetOnChange(&settings.SV, func(ctx context.Context) {
if !fillerStarted && cpuTimeTokenACEnabled.Get(&settings.SV) {
filler.start(ambientCtx.AnnotateCtx(context.Background()))
fillerStarted = true
}
})
}
```
This ticket tracks root causing & fixing the slight negative perf impact of `cpuTimeTokenFiller`. Since the impact is slight, we may not do this until we modify CPU time token AC to support SH & Dedicated, instead of just Serverless.
Make sure to check out https://github.com/cockroachdb/cockroach/pull/161179 for an initial analysis of the regression.
Jira issue: CRDB-59188
Contributor guide
Assessment
This issue has not been assessed yet.