kvserver: OptimisticEvalForLocks alloc regression

[nvanbenschoten]

See #98068 and https://docs.google.com/spreadsheets/d/10GhYr_91CANCNKOM_gPy7Sk9hQkTyQGNgNwNgfHeUtI/edit#gid=4.

benchdiff --old=81a114c --new=master --post-checkout='dev generate go' --run=BenchmarkOptimisticEvalForLocks ./pkg/kv/kvserver

Jira issue: CRDB-25665

[nvanbenschoten]

We only see this regression in allocations on the OptimisticEvalForLocks/real-contention=true variant of the benchmark.

$ benchdiff --old=81a114c --new=master --post-checkout='dev generate go' --run=BenchmarkOptimisticEvalForLocks --count=10 ./pkg/kv/kvserver
test binaries already exist for '81a114c'; skipping build
test binaries already exist for '85c6e38'; skipping build

  pkg=1/1 iter=10/10 cockroachdb/cockroach/pkg/kv/kvserver \

name                                             old time/op    new time/op    delta
OptimisticEvalForLocks/real-contention=true-30     5.30ms ± 0%    5.35ms ± 1%   +0.99%  (p=0.000 n=10+10)
OptimisticEvalForLocks/real-contention=false-30    32.0µs ± 3%    34.4µs ± 1%   +7.39%  (p=0.000 n=10+10)

name                                             old alloc/op   new alloc/op   delta
OptimisticEvalForLocks/real-contention=false-30    8.57kB ± 0%    8.51kB ± 0%   -0.75%  (p=0.000 n=9+8)
OptimisticEvalForLocks/real-contention=true-30     43.9kB ± 3%    52.0kB ± 3%  +18.51%  (p=0.000 n=10+10)

name                                             old allocs/op  new allocs/op  delta
OptimisticEvalForLocks/real-contention=false-30      61.7 ± 1%      60.0 ± 0%   -2.70%  (p=0.000 n=9+8)
OptimisticEvalForLocks/real-contention=true-30        307 ± 1%       370 ± 0%  +20.76%  (p=0.000 n=10+10)

[arulajmani]

@nvanbenschoten and I looked at this together earlier today. We bisected the failure here to #88353, or while comparing mem profiles from 81a114c and 7416aab jumped out to us.

We noticed that the benchmark that's regressing, the real-contention=true variant, sleeps for 500ms. We suspect this is causing background allocations to be counted towards the benchmark results.

We also see vastly different regression amounts (~7% vs. ~18%) when running just the real-contention=true variant and the both the benchmarks respectively.

---

Since the above benchmarks were run, we seem to have regressed further on release-23.1.0.

benchdiff --old=81a114c --post-checkout='dev generate go' --run=BenchmarkOptimisticEvalForLocks --count=10  --new=cockroach/release-23.1.0  ./pkg/kv/kvserver
 
 pkg=1/1 iter=10/10 cockroachdb/cockroach/pkg/kv/kvserver \

name                                             old time/op    new time/op    delta
OptimisticEvalForLocks/real-contention=true-24     5.66ms ± 3%    5.64ms ± 4%     ~     (p=0.796 n=10+10)
OptimisticEvalForLocks/real-contention=false-24    36.6µs ± 4%    39.8µs ± 3%   +8.99%  (p=0.000 n=10+10)

name                                             old alloc/op   new alloc/op   delta
OptimisticEvalForLocks/real-contention=false-24    8.63kB ± 0%    8.14kB ± 0%   -5.76%  (p=0.000 n=9+10)
OptimisticEvalForLocks/real-contention=true-24     45.2kB ± 7%    69.3kB ± 4%  +53.37%  (p=0.000 n=10+9)

name                                             old allocs/op  new allocs/op  delta
OptimisticEvalForLocks/real-contention=false-24      62.3 ± 1%      53.0 ± 0%  -14.97%  (p=0.000 n=9+10)
OptimisticEvalForLocks/real-contention=true-24        313 ± 2%       477 ± 4%  +52.16%  (p=0.000 n=10+9)

However, when diffing memory profiles from 7416aab and f468bd8, we see:

This is unrelated to the benchmark.

---

Given all this, I think we can remove the GA-blocker label on this issue.