Skip to content

test-suites: add a saturated 11-hot-key MSET overwrite spec - #562

Open
fcostaoliveira wants to merge 2 commits into
mainfrom
mset-hot-overwrite-saturated-coverage
Open

fcostaoliveira wants to merge 2 commits into
mainfrom
mset-hot-overwrite-saturated-coverage

Conversation

@fcostaoliveira

@fcostaoliveira fcostaoliveira commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Adds memtier_benchmark-11keys-mset-overwrite-11-pairs-12B-pipeline-10-50_conns.

What it runs

MSET with 11 key/value pairs per command, --pipeline 10 over 50 client connections
(-c 25 -t 2), repeatedly overwriting a hot keyspace of 11 short-named STRING keys holding
12-byte EMBSTR values. Metric of interest is p50 latency (lower is better).

Why, when a 100-key MSET overwrite spec already exists

memtier_benchmark-100keys-mset-overwrite-10B-pipeline-1 covers the same subsystem, but it is
-c 1 -t 1 --pipeline 1 with 3 pairs per command, and it cannot answer a per-command-cost
question. Two independent reasons, both of which I hit in practice:

  1. Concurrency. At one connection and pipeline 1 the server is idle between requests, so
    round-trip time sets the throughput and a few-percent change in the command path is diluted
    below run-to-run noise.
  2. Metric resolution. At that shape p50 comes back at 0.031 ms. memtier reports three
    decimals, so the next representable value is 0.032 — roughly a 3.2% step. An effect smaller
    than one step cannot be expressed at all, and "p50 identical on both sides" is then
    quantization, not evidence of no change. At pipeline 10 over 50 connections p50 sits near
    1.0 ms, where 0.001 ms is about 0.1%.

I ran the -c 1 spec against a reported MSET p50 regression and read its flat result as "no
regression." That was the wrong conclusion from a measurement that could not have shown one.

Key length is deliberate

This scopes redis/redis#15320, which concerns a dictFind key-length fast path, so key-name
lengths are matched to the report rather than normalized: test_MSET1..test_MSET11 gives nine
10-byte names and two 11-byte names, against the report's ten 10-byte and one 11-byte. Using a
uniform-length prefix here would risk tuning out the very path under test.

The 11-key keyspace keeps the main dict and every key object resident in L1/L2 with no
rehashing and effectively no collisions, so dictFind is cache-latency dominated — unlike the
1M–10M distinct-key MSET load specs, which are DRAM-latency dominated and cannot show a
key-comparison difference.

cpus: '3' for -t 2 keeps the client clear of the saturation trap seen in #485/#486/#487.

The -c 1 sibling is left in place for history continuity.

Not yet run on the fleet

This spec has not run on the fleet yet — the preload block follows the same
--key-prefix/--key-minimum/--key-maximum pattern as the merged 100-key sibling, and
check: keyspacelen: 11 will fail the run rather than report a wrong number if it lands short.


Note

Low Risk
Adds only a new benchmark YAML definition; no runtime, auth, or application logic changes.

Overview
Adds a new Redis benchmark suite memtier_benchmark-11keys-mset-overwrite-11-pairs-12B-pipeline-10-50_conns so fleet runs can measure MSET per-command cost on a saturated, 11-key hot overwrite workload with p50 latency as the primary metric.

Compared to the existing low-concurrency 100keys-mset-overwrite-10B-pipeline-1 spec, this one uses pipeline 10 and 50 client connections so the server stays busy and p50 sits around ~1 ms (finer memtier quantization than ~0.031 ms). The preload and workload use test_MSET1..11 with a deliberate mix of 10- and 11-byte key names to align with redis/redis#15320 (dictFind key-length fast path), and keyspacelen: 11 guards dataset shape. Standalone OSS, gcc bookworm amd64/arm64, and dockerhub build variants are included at priority 69.

Reviewed by Cursor Bugbot for commit 2ed4713. Bugbot is set up for automated code reviews on this repo. Configure here.

Adds memtier_benchmark-11keys-mset-overwrite-11-pairs-12B-pipeline-10-50_conns:
MSET with 11 key/value pairs per command, pipeline 10, 50 connections
(-c 25 -t 2), overwriting a hot keyspace of 11 short-named STRING keys with
12-byte EMBSTR values.

The existing memtier_benchmark-100keys-mset-overwrite-10B-pipeline-1 covers the
same subsystem but cannot measure a per-command cost difference: at -c 1 -t 1
--pipeline 1 the round-trip dominates the throughput, and its p50 lands at
0.031 ms where memtier's three-decimal output makes the smallest expressible
step ~3.2% relative. At pipeline 10 over 50 connections p50 is near 1.0 ms, so
0.001 ms is ~0.1% and a few-percent effect is resolvable.

Key-name lengths are matched to the report this scopes (redis/redis#15320,
a dictFind key-length fast path) rather than normalized: test_MSET1..11 yields
nine 10-byte and two 11-byte names.

The -c 1 sibling is left in place for history continuity.
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown

🤖 Automated first-pass review — a human maintainer's review is still required before merge. This is a single read-only pass over the PR description, the diff, and the sibling specs already in the repo; nothing here was run against memtier or the fleet.

The preload's --key-maximum 11 produces ten keys, not eleven, so check: keyspacelen: 11 should fail the run. The preload is --command-key-pattern=P --key-minimum 1 --key-maximum 11 ... -n 11, and the sequential/parallel generator stops one short of --key-maximum — that's the #509 mechanism, and this repo already encodes the workaround in two places. memtier_benchmark-1key-list-10K-elements-lindex-string.yml's own dataset_description says it outright ("preload uses --key-maximum 10001 with -n allkeys, since the S key-pattern stops one short of --key-maximum -- see #509"), and the direct sibling this PR is contrasted against, memtier_benchmark-100keys-mset-overwrite-10B-pipeline-1.yml, uses --key-maximum 101 to land 100 keys. --key-maximum 12 in both the preload and the clientconfig gets you test_MSET1..test_MSET11. Worth fixing before this burns a fleet run — the keyspacelen guard does its job, but it fails after the build and spin-up rather than before.

This also quietly breaks the key-length argument, which the description says is load-bearing: at 1..10 you get nine 10-byte names and one 11-byte name, not the nine/two the dataset_description claims, and one 10-byte key further from the report's ten-and-one.

--command-key-pattern=P on the client side with 50 connections over an 11-key range gives each connection a sub-range smaller than one key. P hands every client its own slice of [key-minimum, key-maximum); with 50 clients and ~10 keys, the slice degenerates, and each of the eleven __key__ placeholders in a single MSET then resolves from that same one-key slice — i.e. one key repeated eleven times per command rather than eleven distinct keys across a mixed length distribution. That's a different dictFind shape than the one redis/redis#15320 describes. Note the 100-key sibling uses --command-key-pattern=R on the MSET client side and reserves =P for its single-client preload; the second commit here moved the client side to P deliberately, so possibly there's a reason I'm not seeing — but if not, R looks like the pattern that actually produces the intended eleven-distinct-key command.

Separately, not a blocker: the description still says -c 25 -t 2 in two places and the PR body still says cpus: '3' for -t 2, while the YAML is -c 5 -t 10 with cpus: '10'. Connection count is unchanged at 50 so the filename is fine, but the prose is describing the pre-second-commit shape. Related, the saturation-trap point the description raises against #485/#486/#487 reads differently at -t 10 on cpus: '10' — 1:1 with no headroom for memtier's own main thread, on a 12-byte MSET at pipeline 10, is exactly the shape where the client can bottleneck first and quietly cap the p50 this spec exists to resolve. Given the whole rationale for the new spec is metric resolution, it'd be worth having the first fleet run report memtier's thread utilization alongside the p50.

Stream field contract and Validate SPEC fields are both green, and nothing in the diff touches stream fields, so category-1 doesn't apply here; the remaining checks were still running when I looked.

Thanks for writing the "why, when a sibling already exists" section — the quantization argument is the right framing and I'd like to see this land. Mainly it's the --key-maximum off-by-one and the client-side key pattern I'd want settled first.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

… pattern

Review follow-up:

- Client sizing. -c 25 -t 2 / cpus 3 concentrated ~430k ops/sec on two memtier
  threads, and every other _conns spec in the suite spreads across 8-10 threads
  with cpus equal to the thread count. If the load generator clips first, a
  server-side delta is masked on both sides -- the same false-negative failure
  mode as -c 1, reached from the client side. Now -c 5 -t 10 / cpus 10, which
  keeps 50 connections exactly.
- Key pattern R -> P. The report writes all 11 distinct keys per command. With R
  drawing with replacement from 11 keys, expected distinct per command is
  ~7.1, so the per-command key-length mix only holds in expectation and the
  varying distinct-lookup count adds spread to the p50 this spec exists to
  resolve finely.
- Documented why --key-maximum equals the key count rather than N+1: the N+1
  preloads elsewhere use -n allkeys, where the count is derived and stops one
  short. With an explicit -n the bound is inclusive, as the merged bitop spec
  (--key-maximum 10, -n 10, keyspacelen 10) demonstrates on the fleet.
- Softened the p50 claim: at 500 outstanding requests p50 is largely queueing
  delay, so it tracks throughput rather than per-command service time and is not
  interchangeable with the -c 1 p50.
@fcostaoliveira

Copy link
Copy Markdown
Collaborator Author

Thanks — the client-thread point was the important one and I've applied it here, plus to the already-merged sibling in #564.

Client sizing: agreed, changed. Now -c 5 -t 10 with cpus: '10', which keeps the connection count exactly and matches what every other _conns spec in the suite does. Your framing is what convinced me: a clipped load generator caps both sides of a comparison equally, so the delta disappears and the run reads as "no regression" — the same false negative these specs exist to prevent, reached from the client side instead of via -c 1. Shipping a fix for one route while leaving another open would defeat the purpose.

@fcostaoliveira

Copy link
Copy Markdown
Collaborator Author

On the two 562-specific points:

Key pattern RP: agreed, changed. Your arithmetic is the argument — ~7.1 expected distinct keys per command out of 11 means roughly a third of the pairs rewrite a key already set earlier in the same command, the nine-10B/two-11B length mix only holds in expectation, and the varying distinct-lookup count adds spread to exactly the p50 this spec exists to resolve finely. P gives each connection test_MSET1..test_MSET11 once per command, which is both the report's shape and the lower-variance option.

--key-maximum 11: staying at 11, and I've documented why in the spec so it doesn't get "fixed" later. You were right to flag it, and right that the N+1 form is the tree-wide pattern — but every one of those preloads uses -n allkeys, which is where the count gets derived from the range and stops one short (#509). With an explicit -n 11 nothing derives the count, and the sequential generator walks key-minimum..key-maximum inclusive.

The empirical check you asked for already exists, on the merged memtier_benchmark-10keys-bitmap-50B-bitop-xor-pipeline-10: it preloads --key-minimum 1 --key-maximum 10 -n 10 against check: keyspacelen: 10, and it has been running on the fleet across both arches for months with real throughput numbers. Since dbconfig_keyspacelen_check runs before the client starts, a 9-key preload there would have failed every run rather than under-loading quietly. So the inclusive-with-explicit--n behaviour is already demonstrated in production.

On the p50 framing: fair, and softened. You're right that at 500 outstanding requests p50 is largely queueing delay against a single-threaded server, so it tracks throughput more than per-command service time. The spec now says so explicitly and warns against reading it as interchangeable with the -c 1 p50.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2ed4713. Configure here.

clientconfig:
run_image: redislabs/memtier_benchmark:edge
tool: memtier_benchmark
arguments: --command="MSET __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__ __key__ __data__" --command-key-pattern=P --key-prefix "test_MSET" --key-minimum 1 --key-maximum 11 --data-size 12 --hide-histogram --test-time 180 --pipeline 10 -c 5 -t 10

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P pattern partitions hot keyspace

High Severity

The benchmark uses --command-key-pattern=P with 50 clients over keys 1–11. Memtier’s P pattern splits that range across clients, so most connections generate keys outside the preloaded set and each MSET repeats a single key. The dict grows, the L1/L2-hot 11-key overwrite path is lost, and p50 no longer measures the dictFind workload this spec claims to capture. The 100-key sibling uses R on the benchmark phase for this reason.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2ed4713. Configure here.

resources:
requests:
cpus: '10'
memory: 2g

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client topology mismatches documented design

Medium Severity

clientconfig runs -c 5 -t 10 with cpus: '10', while the spec text states -c 25 -t 2 and the saturation-trap rationale calls for cpus: '3'. The fleet will record p50 under a 10-thread load-generator shape, not the 2-thread topology this spec documents and was designed around to avoid the #485/#486/#487 client-bound trap.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2ed4713. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant