perf(storage): ZADD reaches its listpack encoding — small zsets stop being skiplists - #793
perf(storage): ZADD reaches its listpack encoding — small zsets stop being skiplists#793TinDang97 wants to merge 1 commit into
Conversation
…being skiplists
`SortedSetListpack` was wired end to end but nothing ever produced one, so every
sorted set was a `skiplist` from its first member. Redis keeps a zset in a
listpack until it exceeds zset-max-listpack-entries (128) or
zset-max-listpack-value (64). Verified against a redis 8.6.1 oracle:
ZADD z 1 a 2 b 3 c -> moon: skiplist redis: listpack
This is the largest per-key memory loss in the campaign: 4722.1 B/key against
redis 7.0.15's 229.9 (20.5x) and 7.4.2's 244.0 (19.4x), the only dimension where
moon loses by more than an order of magnitude.
## Root cause
Identical in shape to #787 (SADD). The listpack guard does not live in
`OwnedKind::upgrade`; it lives one level up, in a per-type accessor the command
layer calls INSTEAD of the owned accessor:
get_or_create_intset EXISTS <- SADD, integer members
get_or_create_hash_listpack EXISTS <- HSET
get_or_create_list_listpack EXISTS <- RPUSH
get_or_create_zset_listpack MISSING
Without it, ZADD went straight to `get_or_create_sorted_set`. The zset case had
a second, sharper edge: `SortedSetKind::upgrade` did NOT upgrade a listpack (a
comment said its upgrade "lives in the zset command layer" — a layer that did
not exist), and `project_mut` only matches `SortedSetBPTree`. So a listpack zset
reaching ANY owned accessor answered WRONGTYPE, not merely "promoted early".
That is why the accessor and the upgrade arm have to land together.
## Change
- `get_or_create_zset_listpack` / `upgrade_zset_listpack_to_bptree`, modelled on
the hash pair including the cold-tier rule (cold storage never persists a
compact encoding, so a promoted value decodes as `SortedSetBPTree` and lands
in the `Ok(None)` fall-through arm rather than being fabricated over).
- `SortedSetKind::upgrade` gains the `SortedSetListpack` arm — the safety net
that keeps every non-ZADD command correct on a key ZADD created compact.
Promotion is one-way, matching Redis.
- ZADD routes through the listpack below both thresholds, honouring
NX/XX/GT/LT/CH in listpack form, and promotes past either with the same
one-time cost-model handoff `hset` and the intset path already perform.
- Scores are stored as `format_score_bytes` output, not the raw argument, so
`ZADD z 3.0 m` answers `ZSCORE` with `3` exactly as the B+tree form does;
Rust's `{}` for f64 is the shortest round-trip-exact rendering, so
`ListpackEntry::as_score` recovers the identical f64 (including inf/-inf).
`as_score` is the single inverse of `format_score_bytes`.
- `parse_zadd_score` is shared by both ZADD paths so the two encodings cannot
diverge on which score arguments they accept.
Reads needed nothing: `SortedSetKind::classify_hot` already answers
`SortedSetRef::Listpack`, and every `_readonly` twin already had an
`entries_sorted()` fallback for it — paths that until now were unreachable.
## Tests
Red/green. Ten new unit tests; the RED test asserts through the real
`OBJECT ENCODING` handler rather than a private field, and 6 of them failed
before the fix with `left: "skiplist", right: "listpack"`.
Three guards mutation-tested — each broken deliberately, shown red, restored:
lp.len()/2 > LISTPACK_MAX_ENTRIES -> usize::MAX
zadd_promotes_past_the_entry_threshold FAILED (listpack != skiplist)
m.len() > LISTPACK_MAX_ELEMENT_SIZE -> usize::MAX
zadd_promotes_on_an_oversized_member FAILED (listpack != skiplist)
SortedSetKind::upgrade listpack arm -> disabled
listpack_zset_upgrades_transparently_on_a_non_zadd_write
FAILED (ZREM answered WRONGTYPE, not 1)
Both threshold tests pass VACUOUSLY before the fix — everything is already a
skiplist — which is exactly why they were mutation-tested.
`test_object_encoding_sorted_set` ASSERTED THE BUG (a one-member zset must be
`skiplist`) and is corrected to expect `listpack`; a new
`test_object_encoding_sorted_set_skiplist_past_threshold` keeps the skiplist
encoding covered. That assertion is why the divergence survived, so 13 zset
OBJECT ENCODING / score-rendering rows are added to scripts/test-consistency.sh
and 6 to scripts/test-commands.sh — no row in either probed zset encoding.
Full lib suite 5118 passed / 0 failed (baseline 5108 + 10 new). clippy clean on
both runtimes with --all-targets; cargo fmt --check clean.
All 17 new rows verified green against a live redis 8.6.1 oracle, plus a
command-by-command A/B: thresholds (128 listpack / 129 skiplist, 64-byte member
listpack / 65-byte skiplist), score normalisation (3.0->3, 1e3->1000,
3.5000->3.5, inf, -inf, 1.0000000000000002), all five ZADD flags, duplicate
in-place update, WRONGTYPE, DUMP/RESTORE round trip, and a kill-and-restart
(the zset comes back as a listpack because the WAL replays ZADD).
## Known limitation — ZADD only
`ZREM`, `ZINCRBY`, `ZPOPMIN`/`ZPOPMAX`, `ZRANGESTORE`, `ZUNIONSTORE` and
`ZINTERSTORE` still reach the value through `get_or_create_sorted_set`, so they
promote a listpack zset to a skiplist. Measured against the oracle:
after ZREM moon: skiplist redis: listpack
after ZINCRBY moon: skiplist redis: listpack
after ZPOPMIN moon: skiplist redis: listpack
after ZADD dup moon: listpack redis: listpack
READS do not promote: ZSCORE, ZCARD, ZRANK, ZRANGE and ZRANGEBYSCORE are served
by `dispatch_read`'s `_readonly` twins, which classify every encoding. Measured
on a live server — all five leave the key a listpack.
So a zset that is only ever added to and read from stays compact — the common
case, and the one the RSS harness measures — while a zset touched by a removal
reverts. That is a strict improvement over "always a skiplist", but it is NOT
full parity, and the encoding now depends on command history.
No consistency row is added for the reverting cases: a row asserting the current
divergent answer would codify the bug, which is precisely the mistake
`test_object_encoding_sorted_set` made and this commit corrects.
Two further gaps found while probing, both PRE-EXISTING at HEAD (confirmed by
reading `git show HEAD:src/command/sorted_set/sorted_set_write.rs` — the same
`get_or_create_sorted_set`-before-validation shape) and deliberately NOT changed
here: `ZADD k XX 1 a` on a missing key leaves an empty zset (`EXISTS` 1 vs
Redis's 0), and a bad score mid-list is applied partially (`ZADD k 1 a nan b`
leaves `a` where Redis validates every score first and writes nothing).
No RSS number is claimed: every published figure must come from a Linux host,
and this was developed on macOS.
Refs #787
author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Directional memory read — macOS, NOT a publishable numberThe PR text says no RSS number is claimed, and that still holds: every published figure Same
~14.9x less memory per zset key, reps agreeing to within 0.7%. Two independent cross-checks say the harness is measuring the right thing:
If the Linux run reproduces this, zset moves from 19.4x vs redis 7.4.2 (244 B/key) to An earlier attempt at this measurement read |
Linux measurement — confirmed. zset 24.50x → 1.58x vs Redis 7.4.2Supersedes the directional macOS comment above. GCE
15.47x less memory per zset key. Against the two oracles, leading with the newer:
Reps agree to ±0.05% (286.2 / 286.4 / 286.5). This closes the single largest per-key memory gap in the campaign — the only dimension where moon lost by more than an order of magnitude. Two provenance notes, because the first attempt at this was wrongThe first Linux run was void and reported The run above was rebuilt through a bundle with a hard guard that Member shape: 5 alphabetic members ( Note this workload is harder than the campaign's earlier zset row (4,431 vs 4,721 B/key baseline, 24.5x vs 19.4x) because 5-member zsets are exactly the shape Redis keeps in a listpack and moon did not. Merge order unchanged#794 (restart preserves compact encodings) must land with or before this, or the win reverts on the first restart. |
1 participant
Root cause
SortedSetListpackwas wired end to end —classify_hotanswered it, every_readonlytwin already had an
entries_sorted()fallback for it — but nothing in the tree everconstructed one that survived, so every sorted set was a
skiplistfrom its firstmember. Redis keeps a zset in a listpack until it exceeds
zset-max-listpack-entries(128) or
zset-max-listpack-value(64).Same shape as #787 (SADD): the listpack guard does not live in
OwnedKind::upgrade, itlives in a per-type accessor the command layer calls instead of the owned accessor, and
get_or_create_zset_listpackdid not exist.The zset case had a second, sharper edge.
SortedSetKind::upgradedid not upgrade alistpack — a comment said its upgrade "lives in the zset command layer", a layer that does
not exist — and
project_mutonly matchesSortedSetBPTree. So a listpack zset reachingany owned accessor answered WRONGTYPE, not merely "promoted early". The accessor and
the upgrade arm therefore had to land together. Mutation-testing proved it: with the arm
disabled,
ZREMon a ZADD-created zset returnsWRONGTYPEinstead of1.Why this one matters most
zset is the single largest per-key memory gap in the campaign — 19.4x vs redis 7.4.2
(4,721 B/key vs 244), the only dimension where moon loses by more than an order of
magnitude.
Score round-trip: verified, not assumed
Scores are stored as
format_score_bytesoutput (Rust's{}for f64 is the shortestround-trip-exact rendering), and
ListpackEntry::as_scoreis its single inverse. Thelistpack re-encodes integral renderings as
Integer, so the i64 boundary is the risk.Probed on a live server, comparing the new listpack path against the existing skiplist
path on identical input (skiplist forced with a 65-byte member) — 14/14 agree:
9007199254740993900719925474099290071992547409929223372036854775807922337203685477600092233720368547760009223372036854775808922337203685477600092233720368547760001e30/-1e301.00000000000000024.9e-324(min subnormal)inf/-inf/-0/0.1/3.0Values above
i64::MAXfall to the String arm (parse::<i64>()returnsNoneonoverflow) and parse back at full precision.
Tests
Red/green: 6 of 10 new tests failed before the fix with
left: "skiplist", right: "listpack". Lib suite 5118 / 0. Three guards mutation-tested (broken → red →restored → green) — necessary because both threshold tests pass vacuously before the
fix, everything already being a skiplist.
test_object_encoding_sorted_setasserted the bug (a one-member zset must beskiplist); corrected, with a new test keeping the skiplist case covered.Verified against a live redis 8.6.1 oracle: 17 new script rows pass, including the
128/129-member and 64/65-byte threshold boundaries, NX/XX/GT/LT/CH, DUMP/RESTORE, and
kill-and-restart.
Known limitations — stated, not hidden
ZREM,ZINCRBY,ZPOPMIN/ZPOPMAX,ZRANGESTORE,ZUNIONSTORE,ZINTERSTOREstill promote toskiplist(measured:after ZREM moon: skiplist redis: listpack). No test codifies the divergence and no consistency row asserts the revertingcases.
dispatch_read's_readonlytwins and leave the key a listpack. The mutable twins would promote and arereachable via shard routing, so this is a property of routing, not a guarantee.
ZADD k XX 1 aon a missingkey leaves an empty zset;
ZADD k 1 a nan bappliesathen errors where Redisvalidates all scores first).
ZADD CHmiscounts sub-epsilon score changes — ZADD CH miscounts sub-epsilon score changes: absolute f64::EPSILON compare where Redis compares exactly #792, pre-existing in the B+treepath; the listpack path deliberately mirrors it so the two encodings agree.
Gates
macOS host: fmt, clippy default, clippy tokio, release lib suite — 4/4. No RSS
number is claimed: every memory figure must come from Linux, and this branch has not
been measured there yet. The 19.4x above is main's measured gap, not a post-fix claim.
Refs #787
author: Tin Dang