fix(persistence): a restart no longer flattens every compact encoding - #794
Closed
TinDang97 wants to merge 1 commit into
Closed
fix(persistence): a restart no longer flattens every compact encoding#794TinDang97 wants to merge 1 commit into
TinDang97 wants to merge 1 commit into
Conversation
RDB decode rebuilt every container in its full form, so a listpack hash, a listpack list, an intset and a set listpack all came back as hashtable/linkedlist/hashtable on the first reload. Redis preserves all of them across DEBUG RELOAD. The memory cost is not incidental. The SADD-builds-a-listpack win (#787, set 404.5 B/key) reverted to 978.2 B/key -- 1.85x back to 4.48x vs redis 7.4.2 -- the moment the server came back up, which makes every small-container encoding win in this campaign conditional on this fix. ## The trap: two decoders, and the obvious one is not the one that runs `src/persistence/rdb.rs` has TWO decode paths. `read_entry_zero_copy` is a second, hand-rolled decoder that builds each container inline and never calls `value_codec::decode_value_body`. Hooking `decode_value_body` alone therefore changed nothing observable: the restart path -- including `load_from_bytes`, the AOF RDB preamble -- goes through `read_entry_zero_copy`, and the guard test still failed with all four types flattened. Both funnels are now covered. ## Change - `value_codec::compact_after_decode` re-derives the compact encoding against thresholds already in the tree (LISTPACK_MAX_ENTRIES, LISTPACK_MAX_ELEMENT_SIZE, INTSET_MAX_ENTRIES). - Applied at `read_entry_zero_copy`'s single Entry funnel and, via `decode_value_body_compacting`, at the `value_codec` path. - No wire-format change: every listpack variant already maps to the same `ValueType` tag as its full form (`value_type_of`), so files stay readable in both directions. They are not byte-identical -- a listpack preserves insertion order where a HashMap does not. - `HashWithTtl` is deliberately not compacted: a listpack carries no TTL sidecar. ## Deliberately NOT the cold/spill path `ValueKind::classify_cold` accepts only the canonical full forms, so a cold-decoded `SetListpack` falls through its `_ => Err(WrongType)` arm and answers WRONGTYPE for a perfectly valid set. The first attempt compacted the shared `decode_value_body` and turned 11 cold-tier tests red for exactly that reason. Compaction is therefore opt-in, RDB-path only. Widening `classify_cold` so the cold tier compacts too is a named follow-up, as is the `redis_rdb.rs` read side used by DUMP/RESTORE and replica full sync. ## Tests `tests/restart_preserves_compact_encoding.rs` writes one key of each compact type plus a 200-field hash as a negative control -- without it, a bug that compacted EVERYTHING would pass -- restarts a real server on the same --dir, and asserts the target encoding of all five. It asserts the post-restart encoding absolutely rather than before == after, so it is meaningful on main, where a small string set legitimately gains a listpack here. Three existing tests asserted the OLD behaviour and are corrected: persistence::rdb::tests::test_round_trip_{hash,list,set} matched RedisValueRef::Hash/List/Set and panicked "Expected hash" the moment decode started returning the compact form. They now assert the listpack variant AND keep every data assertion, so they check strictly more than before -- the encoding as well as the contents. Lib suite 5137 passed / 0 failed. 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 |
Collaborator
Author
Three existing tests asserted the old behaviour
They now assert the listpack variant and keep every data assertion, so they check strictly more than before: encoding and contents, including that list order survives the re-derivation. Local gate on the final tree: fmt, clippy |
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Sep 1, 2026
TinDang97
added a commit
that referenced
this pull request
Sep 7, 2026
…re; pin the zset exclusion Fix-forward of #794 after the rebase onto v0.8.9 (6251429), per the adversarial review. Compile. The branch merged textually clean and did not compile: #803 made `SetValue = indexmap::IndexSet<Bytes>` and deleted `Listpack::to_hash_set`, and the new unit tests built a `HashSet<Bytes>` for `RedisValueRef::Set`. Four errors, all in test code. They now use `SetValue` and `to_set_value()`. Both runtimes check clean with `--all-targets`. Gate. `tests/restart_preserves_compact_encoding.rs` was `#[ignore]`d, and every `--ignored` invocation in `.github/workflows/` names a specific `--test` target, so the headline regression test ran nowhere. The `#[ignore]` is dropped: the test spawns one server twice on a reserved port in a unique `--dir`, exactly like ~60 other un-ignored suites under `tests/`, so it now runs in every leg that runs `cargo nextest run` / `cargo test` (hosted tokio Check, self-hosted monoio, both VM suites of `scripts/ci-local.sh`). The fixed 3 s sleep after `BGREWRITEAOF` is replaced with a wait for the AOF manifest `seq` to advance and the new `moon.aof.<seq>.base.rdb` to exist -- the command acks at enqueue, so `aof_rewrite_in_progress:0` can be observed before the rewrite starts, and `aof_base_size` in `INFO persistence` does not move when it finishes (measured 66 before and after a rewrite that wrote a 3 KB base; that INFO field is its own small bug, not fixed here). The reply is asserted too, so a refused rewrite fails loudly instead of "passing" on a command-log replay. Proven red against a pre-fix binary and green with the fix (outputs in tmp/perf-campaign/FIXFWD-819-794.md). Probe. The test asserts through `OBJECT ENCODING`, which reads `entry.value` via `Database::get` / `get_if_alive_any_plane` on both dispatch paths -- neither routes through `get_promoted`, so the probe cannot itself flatten the key (moon#832). Stated in the test header so nobody "improves" it into a promoting accessor. Zsets. The review asked whether the exclusion makes #793's 20.5x win restart-transient. It does -- and the arm cannot land here: `SortedSetKind::project_mut` / `project_ref` accept only `SortedSetBPTree`, and `SortedSetKind::upgrade` deliberately leaves `SortedSetListpack` alone, so a zset compacted on reload would answer WRONGTYPE to every zset command on the mutable path, `ZADD` included. `SortedSetListpack` is unreachable from every load path today (`value_codec`, `redis_rdb`, DUMP/RESTORE all rebuild the full form), so the arm would create that hazard rather than inherit it. A unit test pins the exclusion (`small_zset_is_left_in_full_form_until_the_write_ path_accepts_a_listpack`) so that lifting it is a decision taken by the change that adds the write-side upgrade arm. `INTSET_MAX_ENTRIES` (512) had two private definitions and a comment promising they agree; it is now one `pub const` in `storage::db`, read by both the `SADD` path and the decode-side re-derivation. CHANGELOG: the rebase carried the entry into the [0.8.9] section; it is moved back under [Unreleased] and updated to describe the above. Refs #787, #832 author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 7, 2026
…#840) * fix(persistence): a restart no longer flattens every compact encoding RDB decode rebuilt every container in its full form, so a listpack hash, a listpack list, an intset and a set listpack all came back as hashtable/linkedlist/hashtable on the first reload. Redis preserves all of them across DEBUG RELOAD. The memory cost is not incidental. The SADD-builds-a-listpack win (#787, set 404.5 B/key) reverted to 978.2 B/key -- 1.85x back to 4.48x vs redis 7.4.2 -- the moment the server came back up, which makes every small-container encoding win in this campaign conditional on this fix. ## The trap: two decoders, and the obvious one is not the one that runs `src/persistence/rdb.rs` has TWO decode paths. `read_entry_zero_copy` is a second, hand-rolled decoder that builds each container inline and never calls `value_codec::decode_value_body`. Hooking `decode_value_body` alone therefore changed nothing observable: the restart path -- including `load_from_bytes`, the AOF RDB preamble -- goes through `read_entry_zero_copy`, and the guard test still failed with all four types flattened. Both funnels are now covered. ## Change - `value_codec::compact_after_decode` re-derives the compact encoding against thresholds already in the tree (LISTPACK_MAX_ENTRIES, LISTPACK_MAX_ELEMENT_SIZE, INTSET_MAX_ENTRIES). - Applied at `read_entry_zero_copy`'s single Entry funnel and, via `decode_value_body_compacting`, at the `value_codec` path. - No wire-format change: every listpack variant already maps to the same `ValueType` tag as its full form (`value_type_of`), so files stay readable in both directions. They are not byte-identical -- a listpack preserves insertion order where a HashMap does not. - `HashWithTtl` is deliberately not compacted: a listpack carries no TTL sidecar. ## Deliberately NOT the cold/spill path `ValueKind::classify_cold` accepts only the canonical full forms, so a cold-decoded `SetListpack` falls through its `_ => Err(WrongType)` arm and answers WRONGTYPE for a perfectly valid set. The first attempt compacted the shared `decode_value_body` and turned 11 cold-tier tests red for exactly that reason. Compaction is therefore opt-in, RDB-path only. Widening `classify_cold` so the cold tier compacts too is a named follow-up, as is the `redis_rdb.rs` read side used by DUMP/RESTORE and replica full sync. ## Tests `tests/restart_preserves_compact_encoding.rs` writes one key of each compact type plus a 200-field hash as a negative control -- without it, a bug that compacted EVERYTHING would pass -- restarts a real server on the same --dir, and asserts the target encoding of all five. It asserts the post-restart encoding absolutely rather than before == after, so it is meaningful on main, where a small string set legitimately gains a listpack here. Three existing tests asserted the OLD behaviour and are corrected: persistence::rdb::tests::test_round_trip_{hash,list,set} matched RedisValueRef::Hash/List/Set and panicked "Expected hash" the moment decode started returning the compact form. They now assert the listpack variant AND keep every data assertion, so they check strictly more than before -- the encoding as well as the contents. Lib suite 5137 passed / 0 failed. Refs #787 author: Tin Dang * fix(persistence): make the restart-encoding gate compile, run, and fire; pin the zset exclusion Fix-forward of #794 after the rebase onto v0.8.9 (6251429), per the adversarial review. Compile. The branch merged textually clean and did not compile: #803 made `SetValue = indexmap::IndexSet<Bytes>` and deleted `Listpack::to_hash_set`, and the new unit tests built a `HashSet<Bytes>` for `RedisValueRef::Set`. Four errors, all in test code. They now use `SetValue` and `to_set_value()`. Both runtimes check clean with `--all-targets`. Gate. `tests/restart_preserves_compact_encoding.rs` was `#[ignore]`d, and every `--ignored` invocation in `.github/workflows/` names a specific `--test` target, so the headline regression test ran nowhere. The `#[ignore]` is dropped: the test spawns one server twice on a reserved port in a unique `--dir`, exactly like ~60 other un-ignored suites under `tests/`, so it now runs in every leg that runs `cargo nextest run` / `cargo test` (hosted tokio Check, self-hosted monoio, both VM suites of `scripts/ci-local.sh`). The fixed 3 s sleep after `BGREWRITEAOF` is replaced with a wait for the AOF manifest `seq` to advance and the new `moon.aof.<seq>.base.rdb` to exist -- the command acks at enqueue, so `aof_rewrite_in_progress:0` can be observed before the rewrite starts, and `aof_base_size` in `INFO persistence` does not move when it finishes (measured 66 before and after a rewrite that wrote a 3 KB base; that INFO field is its own small bug, not fixed here). The reply is asserted too, so a refused rewrite fails loudly instead of "passing" on a command-log replay. Proven red against a pre-fix binary and green with the fix (outputs in tmp/perf-campaign/FIXFWD-819-794.md). Probe. The test asserts through `OBJECT ENCODING`, which reads `entry.value` via `Database::get` / `get_if_alive_any_plane` on both dispatch paths -- neither routes through `get_promoted`, so the probe cannot itself flatten the key (moon#832). Stated in the test header so nobody "improves" it into a promoting accessor. Zsets. The review asked whether the exclusion makes #793's 20.5x win restart-transient. It does -- and the arm cannot land here: `SortedSetKind::project_mut` / `project_ref` accept only `SortedSetBPTree`, and `SortedSetKind::upgrade` deliberately leaves `SortedSetListpack` alone, so a zset compacted on reload would answer WRONGTYPE to every zset command on the mutable path, `ZADD` included. `SortedSetListpack` is unreachable from every load path today (`value_codec`, `redis_rdb`, DUMP/RESTORE all rebuild the full form), so the arm would create that hazard rather than inherit it. A unit test pins the exclusion (`small_zset_is_left_in_full_form_until_the_write_ path_accepts_a_listpack`) so that lifting it is a decision taken by the change that adds the write-side upgrade arm. `INTSET_MAX_ENTRIES` (512) had two private definitions and a comment promising they agree; it is now one `pub const` in `storage::db`, read by both the `SADD` path and the decode-side re-derivation. CHANGELOG: the rebase carried the entry into the [0.8.9] section; it is moved back under [Unreleased] and updated to describe the above. Refs #787, #832 author: Tin Dang * test(persistence): make the restart-encoding gate wait on either AOF layout The gate un-ignored by this branch hung for its full 30s timeout on the hosted tokio Check leg — TRY 3 FAIL, deterministic, not a flake — while passing on monoio. It waited on the `appendonlydir` manifest `seq`, which the tokio TopLevel writer never creates: it appends to one flat `<dir>/appendonly.aof` instead (src/persistence/aof/auto_rewrite.rs:59-62, "they never coexist for one server"). `manifest_seq` therefore returned None forever and the wait could only time out. Confirmed on disk: a tokio server with --appendonly yes writes `<dir>/appendonly.aof` and no `appendonlydir` at all. Replaces the manifest-only read with a layout-independent `Base` fingerprint — manifest `seq` where that layout exists, otherwise the flat file's (len, mtime), which a rewrite replaces wholesale. The manifest arm keeps its existing requirement that the published `moon.aof.<seq>.base.rdb` also exists, since the seq line lands before the file is fsynced into place. The gate still discriminates. Against an unfixed origin/main binary it fails in 0.56s naming every flattened encoding — "h: was listpack, after restart hashtable ... si: was intset, after restart hashtable" — rather than timing out, so a future regression is reported as itself and not as a hang. tokio 4.42s pass, monoio 5.09s pass, unfixed main FAILED as designed.
Collaborator
Author
|
Superseded — landed as #840 ( |
TinDang97
added a commit
that referenced
this pull request
Sep 7, 2026
…s stop being hashtables
`SetListpack` existed as a storage encoding and `SADD` already had an intset
path, but nothing ever CREATED a string-set listpack: a small set of
non-integer members went straight to `hashtable`, where Redis keeps one in a
listpack up to set-max-listpack-entries (128) / set-max-listpack-value (64).
The guard does not live in `OwnedKind::upgrade` (every impl is unconditional)
-- it lives one level up, in the per-type accessor the command layer calls
INSTEAD of the owned accessor, and the set one was missing:
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_set_listpack MISSING
Rework of the original branch against main @6251429f, which had moved under
it in three ways the adversarial review named:
- moon#803 made `SetValue = IndexSet<Bytes>` and deleted
`Listpack::to_hash_set()`; the branch returned `&mut HashSet<Bytes>` and
could not compile. `upgrade_set_listpack_to_set` now returns `&mut SetValue`
and delegates to `SetKind::upgrade` -- the same conversion `get_or_create`
and `get_promoted` run -- applying the returned delta to `used_memory`
itself, so the listpack -> IndexSet swing has one implementation.
- moon#810 bills the IndexSet from its real capacity (`set_table_bytes`); the
branch's promotion charged only per-member bytes and would have
under-counted the whole table. Delegating to `SetKind::upgrade` makes that
impossible by construction; `sadd_listpack_path_keeps_the_ledger_exact_
through_promotion` walks create / duplicate / promote / delete against a
full recompute.
- moon#801 removed the per-entry allocation from every listpack lookup; the
branch's `lp.iter().any(|m| m.as_bytes() == member)` put it back, one Vec
per entry walked, in a file under the hot-path allocation ban. The scan is
`Listpack::contains_element` -- the borrowed comparison #801 added for
exactly this lookup. A non-bulk argument is skipped, as the standard path
does, never returned from inside the charge window (moon#814 shape).
The misleading test is corrected. `listpack_set_answers_reads_identically`
read through the MUTABLE `scard`/`sismember`, which are `get_promoted` and
flatten the listpack on the first call -- so from its second assertion on it
was testing a hashtable. It now reads through the `_readonly` twins, checks
the VALUE (SMEMBERS, an integer-encoded member answering to its decimal
spelling) and not just the encoding, and re-asserts `listpack` after every
read. Both threshold tests now assert the AT-threshold half too (128 members,
a 64-byte member), which the pre-fix binary answers `hashtable` to, so
neither can pass vacuously.
Ceiling, stated rather than sold (moon#832): `get_set` is `get_promoted`,
which upgrades unconditionally, and nothing downgrades. Any set command on
the mutable dispatch path -- SREM, SPOP, SMOVE, the store commands, every
command inside MULTI/EXEC or Lua, and the reads whenever they take that path
-- flattens the listpack on first touch. This is a write-only-workload win.
An intset that receives a string member still promotes straight to
`hashtable` (Redis 7.2+ makes it a listpack); named residual of moon#787.
Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh
server per row, 200k keys x `SADD s:i alpha beta gamma`, RSS delta per key,
2 reps): 562 -> 264 B/key (-53%). Redis 7.4.2 measures 106 B/key on the same
probe; the remaining gap is moon's per-key envelope (CompactValue slot +
Box<RedisValue> + the listpack Vec header), not the encoding. The author's
original figure (GCE c3-standard-8: 978.2 -> 404.5 B/key) was a different
probe and is superseded by this one.
Refs moon#787, moon#832. Committed on top of fix/restart-preserves-compact-encoding
(#794 @14b0db85) and lands after it -- without it every set listpack a restart touches is flattened and the
win is a fresh-server artifact; with #794 and without this, a small string
set is `hashtable` live and `listpack` after reload.
author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 7, 2026
…s it `ZADD` reported `skiplist` from its first member where Redis keeps a zset in a listpack up to zset-max-listpack-entries (128) / zset-max-listpack-value (64). `SortedSetListpack` was wired end to end -- the value codec, both RDB writers, the AOF rewriter, DEBUG DIGEST, MEMORY USAGE, and the read-only `SortedSetRef::Listpack` arm all handled it -- but no accessor ever produced one, so every zset paid the full B+tree-plus-HashMap cost (the 20.5x deficit in moon#787). ZADD now routes through `get_or_create_zset_listpack` below both thresholds and promotes past either; `SortedSetKind::upgrade` gains the listpack arm that keeps every other zset command correct on a key ZADD created compact. Rework of the original branch, stacked on perf/set-listpack-encoding (the SADD rework) on top of fix/restart-preserves-compact-encoding @14b0db85 (itself on main @6251429f), so the three land in that order without a conflict. The blocking finding of the adversarial review is fixed structurally, not patched: - The branch inserted its listpack path ABOVE the moon#814/#820 validation pre-pass, created the key, then parsed scores inside the mutation loop with `return e` on failure -- skipping the charge. `ZADD z 1 a 2 b notafloat c` left `a` and `b` written, uncharged, behind an error reply: the exact regression f2fe28a had merged three days earlier, on the new path. The branch now sits BELOW the pre-pass and above `get_or_create_sorted_set`, so every pair is proven parseable before the keyspace is touched and an erroring ZADD creates no key and writes no prefix. `zadd_that_errors_mid_command_on_the_listpack_path_keeps_the_ledger_exact` and `zadd_listpack_path_is_all_or_nothing_on_a_bad_score` pin both halves; both fail against the submitted shape. - moon#810 bills the arena and members table from real capacity; the promotion now delegates to `SortedSetKind::upgrade` and applies its delta, so the swing has one implementation and one cost model. `zadd_listpack_path_keeps_the_ledger_exact_through_promotion` walks create / in-place update (longer and shorter rendering) / promote / delete. - The member lookup was `iter_pairs()` + `as_bytes()`: two allocations per pair walked, under the hot-path ban (moon#801). It is now a borrowed `iter_pair_refs` scan; the score is decoded from the `ListpackRef` without materialising it. - Scores are rendered into a stack `ScoreBuf` (`storage::zset_score`), not a heap `Bytes` per pair. `render_score` is byte-identical to the command layer's `format_score_bytes` and round-trip exact through `parse_score`; both properties are pinned by tests. The module lives in storage because the decode side renders too. Restart. #794 deliberately excluded zsets from `compact_after_decode` and pinned the exclusion with a tripwire, because a reloaded listpack would have answered WRONGTYPE on the mutable path until the upgrade arm existed. This change is that arm, so it lifts the tripwire in the same commit: the zset arm re-derives a listpack from the B+tree in score order, the tripwire becomes `small_zset_round_trips_back_to_listpack_with_scores_intact` (asserting every member AND every score, including -1.5 and 0.1+0.2), two negative controls keep the past-threshold and oversized cases `skiplist`, and the live restart gate gains a zset key plus a post-restart ZSCORE of its non-integral score. Ceiling, stated rather than sold (moon#832): ZADD is the only zset command that mutates a listpack in place. ZREM, ZINCRBY, ZPOPMIN/ZPOPMAX, the store commands, every zset command inside MULTI/EXEC or Lua, and the reads not in `dispatch_read` at all (ZRANGEBYLEX, ZREVRANGEBYLEX, ZRANDMEMBER, ZINTERCARD) reach the value through `get_promoted` and flatten it on first touch. This is a write-only-workload win. `ZADD z XX 1 a` on a missing key still leaves an empty zset (the moon#830 class, one more site). The CH count keeps main's absolute-EPSILON comparison, now in two places. Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh server per row, 200k keys x `ZADD z:i 1 alpha 2.5 beta 3 gamma`, RSS delta per key, 2 reps): 4165 -> 295 B/key (-93%, 14x). Redis 7.4.2 measures 108 B/key on the same probe; the remaining gap is moon's per-key envelope, not the encoding. Refs moon#787, moon#832, moon#830. Must land AFTER #794 (it edits #794's test and decode arm) and is committed on top of #791; it cannot land before either. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s stop being hashtables
`SetListpack` existed as a storage encoding and `SADD` already had an intset
path, but nothing ever CREATED a string-set listpack: a small set of
non-integer members went straight to `hashtable`, where Redis keeps one in a
listpack up to set-max-listpack-entries (128) / set-max-listpack-value (64).
The guard does not live in `OwnedKind::upgrade` (every impl is unconditional)
-- it lives one level up, in the per-type accessor the command layer calls
INSTEAD of the owned accessor, and the set one was missing:
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_set_listpack MISSING
Rework of the original branch against main @6251429f, which had moved under
it in three ways the adversarial review named:
- moon#803 made `SetValue = IndexSet<Bytes>` and deleted
`Listpack::to_hash_set()`; the branch returned `&mut HashSet<Bytes>` and
could not compile. `upgrade_set_listpack_to_set` now returns `&mut SetValue`
and delegates to `SetKind::upgrade` -- the same conversion `get_or_create`
and `get_promoted` run -- applying the returned delta to `used_memory`
itself, so the listpack -> IndexSet swing has one implementation.
- moon#810 bills the IndexSet from its real capacity (`set_table_bytes`); the
branch's promotion charged only per-member bytes and would have
under-counted the whole table. Delegating to `SetKind::upgrade` makes that
impossible by construction; `sadd_listpack_path_keeps_the_ledger_exact_
through_promotion` walks create / duplicate / promote / delete against a
full recompute.
- moon#801 removed the per-entry allocation from every listpack lookup; the
branch's `lp.iter().any(|m| m.as_bytes() == member)` put it back, one Vec
per entry walked, in a file under the hot-path allocation ban. The scan is
`Listpack::contains_element` -- the borrowed comparison #801 added for
exactly this lookup. A non-bulk argument is skipped, as the standard path
does, never returned from inside the charge window (moon#814 shape).
The misleading test is corrected. `listpack_set_answers_reads_identically`
read through the MUTABLE `scard`/`sismember`, which are `get_promoted` and
flatten the listpack on the first call -- so from its second assertion on it
was testing a hashtable. It now reads through the `_readonly` twins, checks
the VALUE (SMEMBERS, an integer-encoded member answering to its decimal
spelling) and not just the encoding, and re-asserts `listpack` after every
read. Both threshold tests now assert the AT-threshold half too (128 members,
a 64-byte member), which the pre-fix binary answers `hashtable` to, so
neither can pass vacuously.
Ceiling, stated rather than sold (moon#832): `get_set` is `get_promoted`,
which upgrades unconditionally, and nothing downgrades. Any set command on
the mutable dispatch path -- SREM, SPOP, SMOVE, the store commands, every
command inside MULTI/EXEC or Lua, and the reads whenever they take that path
-- flattens the listpack on first touch. This is a write-only-workload win.
An intset that receives a string member still promotes straight to
`hashtable` (Redis 7.2+ makes it a listpack); named residual of moon#787.
Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh
server per row, 200k keys x `SADD s:i alpha beta gamma`, RSS delta per key,
2 reps): 562 -> 264 B/key (-53%). Redis 7.4.2 measures 106 B/key on the same
probe; the remaining gap is moon's per-key envelope (CompactValue slot +
Box<RedisValue> + the listpack Vec header), not the encoding. The author's
original figure (GCE c3-standard-8: 978.2 -> 404.5 B/key) was a different
probe and is superseded by this one.
Refs moon#787, moon#832. Committed on top of fix/restart-preserves-compact-encoding
(#794 @14b0db85) and lands after it -- without it every set listpack a restart touches is flattened and the
win is a fresh-server artifact; with #794 and without this, a small string
set is `hashtable` live and `listpack` after reload.
author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s stop being hashtables
`SetListpack` existed as a storage encoding and `SADD` already had an intset
path, but nothing ever CREATED a string-set listpack: a small set of
non-integer members went straight to `hashtable`, where Redis keeps one in a
listpack up to set-max-listpack-entries (128) / set-max-listpack-value (64).
The guard does not live in `OwnedKind::upgrade` (every impl is unconditional)
-- it lives one level up, in the per-type accessor the command layer calls
INSTEAD of the owned accessor, and the set one was missing:
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_set_listpack MISSING
Rework of the original branch against main @6251429f, which had moved under
it in three ways the adversarial review named:
- moon#803 made `SetValue = IndexSet<Bytes>` and deleted
`Listpack::to_hash_set()`; the branch returned `&mut HashSet<Bytes>` and
could not compile. `upgrade_set_listpack_to_set` now returns `&mut SetValue`
and delegates to `SetKind::upgrade` -- the same conversion `get_or_create`
and `get_promoted` run -- applying the returned delta to `used_memory`
itself, so the listpack -> IndexSet swing has one implementation.
- moon#810 bills the IndexSet from its real capacity (`set_table_bytes`); the
branch's promotion charged only per-member bytes and would have
under-counted the whole table. Delegating to `SetKind::upgrade` makes that
impossible by construction; `sadd_listpack_path_keeps_the_ledger_exact_
through_promotion` walks create / duplicate / promote / delete against a
full recompute.
- moon#801 removed the per-entry allocation from every listpack lookup; the
branch's `lp.iter().any(|m| m.as_bytes() == member)` put it back, one Vec
per entry walked, in a file under the hot-path allocation ban. The scan is
`Listpack::contains_element` -- the borrowed comparison #801 added for
exactly this lookup. A non-bulk argument is skipped, as the standard path
does, never returned from inside the charge window (moon#814 shape).
The misleading test is corrected. `listpack_set_answers_reads_identically`
read through the MUTABLE `scard`/`sismember`, which are `get_promoted` and
flatten the listpack on the first call -- so from its second assertion on it
was testing a hashtable. It now reads through the `_readonly` twins, checks
the VALUE (SMEMBERS, an integer-encoded member answering to its decimal
spelling) and not just the encoding, and re-asserts `listpack` after every
read. Both threshold tests now assert the AT-threshold half too (128 members,
a 64-byte member), which the pre-fix binary answers `hashtable` to, so
neither can pass vacuously.
Ceiling, stated rather than sold (moon#832): `get_set` is `get_promoted`,
which upgrades unconditionally, and nothing downgrades. Any set command on
the mutable dispatch path -- SREM, SPOP, SMOVE, the store commands, every
command inside MULTI/EXEC or Lua, and the reads whenever they take that path
-- flattens the listpack on first touch. This is a write-only-workload win.
An intset that receives a string member still promotes straight to
`hashtable` (Redis 7.2+ makes it a listpack); named residual of moon#787.
Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh
server per row, 200k keys x `SADD s:i alpha beta gamma`, RSS delta per key,
2 reps): 562 -> 264 B/key (-53%). Redis 7.4.2 measures 106 B/key on the same
probe; the remaining gap is moon's per-key envelope (CompactValue slot +
Box<RedisValue> + the listpack Vec header), not the encoding. The author's
original figure (GCE c3-standard-8: 978.2 -> 404.5 B/key) was a different
probe and is superseded by this one.
Refs moon#787, moon#832. Committed on top of fix/restart-preserves-compact-encoding
(#794 @14b0db85) and lands after it -- without it every set listpack a restart touches is flattened and the
win is a fresh-server artifact; with #794 and without this, a small string
set is `hashtable` live and `listpack` after reload.
author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s it `ZADD` reported `skiplist` from its first member where Redis keeps a zset in a listpack up to zset-max-listpack-entries (128) / zset-max-listpack-value (64). `SortedSetListpack` was wired end to end -- the value codec, both RDB writers, the AOF rewriter, DEBUG DIGEST, MEMORY USAGE, and the read-only `SortedSetRef::Listpack` arm all handled it -- but no accessor ever produced one, so every zset paid the full B+tree-plus-HashMap cost (the 20.5x deficit in moon#787). ZADD now routes through `get_or_create_zset_listpack` below both thresholds and promotes past either; `SortedSetKind::upgrade` gains the listpack arm that keeps every other zset command correct on a key ZADD created compact. Rework of the original branch, stacked on perf/set-listpack-encoding (the SADD rework) on top of fix/restart-preserves-compact-encoding @14b0db85 (itself on main @6251429f), so the three land in that order without a conflict. The blocking finding of the adversarial review is fixed structurally, not patched: - The branch inserted its listpack path ABOVE the moon#814/#820 validation pre-pass, created the key, then parsed scores inside the mutation loop with `return e` on failure -- skipping the charge. `ZADD z 1 a 2 b notafloat c` left `a` and `b` written, uncharged, behind an error reply: the exact regression f2fe28a had merged three days earlier, on the new path. The branch now sits BELOW the pre-pass and above `get_or_create_sorted_set`, so every pair is proven parseable before the keyspace is touched and an erroring ZADD creates no key and writes no prefix. `zadd_that_errors_mid_command_on_the_listpack_path_keeps_the_ledger_exact` and `zadd_listpack_path_is_all_or_nothing_on_a_bad_score` pin both halves; both fail against the submitted shape. - moon#810 bills the arena and members table from real capacity; the promotion now delegates to `SortedSetKind::upgrade` and applies its delta, so the swing has one implementation and one cost model. `zadd_listpack_path_keeps_the_ledger_exact_through_promotion` walks create / in-place update (longer and shorter rendering) / promote / delete. - The member lookup was `iter_pairs()` + `as_bytes()`: two allocations per pair walked, under the hot-path ban (moon#801). It is now a borrowed `iter_pair_refs` scan; the score is decoded from the `ListpackRef` without materialising it. - Scores are rendered into a stack `ScoreBuf` (`storage::zset_score`), not a heap `Bytes` per pair. `render_score` is byte-identical to the command layer's `format_score_bytes` and round-trip exact through `parse_score`; both properties are pinned by tests. The module lives in storage because the decode side renders too. Restart. #794 deliberately excluded zsets from `compact_after_decode` and pinned the exclusion with a tripwire, because a reloaded listpack would have answered WRONGTYPE on the mutable path until the upgrade arm existed. This change is that arm, so it lifts the tripwire in the same commit: the zset arm re-derives a listpack from the B+tree in score order, the tripwire becomes `small_zset_round_trips_back_to_listpack_with_scores_intact` (asserting every member AND every score, including -1.5 and 0.1+0.2), two negative controls keep the past-threshold and oversized cases `skiplist`, and the live restart gate gains a zset key plus a post-restart ZSCORE of its non-integral score. Ceiling, stated rather than sold (moon#832): ZADD is the only zset command that mutates a listpack in place. ZREM, ZINCRBY, ZPOPMIN/ZPOPMAX, the store commands, every zset command inside MULTI/EXEC or Lua, and the reads not in `dispatch_read` at all (ZRANGEBYLEX, ZREVRANGEBYLEX, ZRANDMEMBER, ZINTERCARD) reach the value through `get_promoted` and flatten it on first touch. This is a write-only-workload win. `ZADD z XX 1 a` on a missing key still leaves an empty zset (the moon#830 class, one more site). The CH count keeps main's absolute-EPSILON comparison, now in two places. Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh server per row, 200k keys x `ZADD z:i 1 alpha 2.5 beta 3 gamma`, RSS delta per key, 2 reps): 4165 -> 295 B/key (-93%, 14x). Redis 7.4.2 measures 108 B/key on the same probe; the remaining gap is moon's per-key envelope, not the encoding. Refs moon#787, moon#832, moon#830. Must land AFTER #794 (it edits #794's test and decode arm) and is committed on top of #791; it cannot land before either. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s stop being hashtables
`SetListpack` existed as a storage encoding and `SADD` already had an intset
path, but nothing ever CREATED a string-set listpack: a small set of
non-integer members went straight to `hashtable`, where Redis keeps one in a
listpack up to set-max-listpack-entries (128) / set-max-listpack-value (64).
The guard does not live in `OwnedKind::upgrade` (every impl is unconditional)
-- it lives one level up, in the per-type accessor the command layer calls
INSTEAD of the owned accessor, and the set one was missing:
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_set_listpack MISSING
Rework of the original branch against main @6251429f, which had moved under
it in three ways the adversarial review named:
- moon#803 made `SetValue = IndexSet<Bytes>` and deleted
`Listpack::to_hash_set()`; the branch returned `&mut HashSet<Bytes>` and
could not compile. `upgrade_set_listpack_to_set` now returns `&mut SetValue`
and delegates to `SetKind::upgrade` -- the same conversion `get_or_create`
and `get_promoted` run -- applying the returned delta to `used_memory`
itself, so the listpack -> IndexSet swing has one implementation.
- moon#810 bills the IndexSet from its real capacity (`set_table_bytes`); the
branch's promotion charged only per-member bytes and would have
under-counted the whole table. Delegating to `SetKind::upgrade` makes that
impossible by construction; `sadd_listpack_path_keeps_the_ledger_exact_
through_promotion` walks create / duplicate / promote / delete against a
full recompute.
- moon#801 removed the per-entry allocation from every listpack lookup; the
branch's `lp.iter().any(|m| m.as_bytes() == member)` put it back, one Vec
per entry walked, in a file under the hot-path allocation ban. The scan is
`Listpack::contains_element` -- the borrowed comparison #801 added for
exactly this lookup. A non-bulk argument is skipped, as the standard path
does, never returned from inside the charge window (moon#814 shape).
The misleading test is corrected. `listpack_set_answers_reads_identically`
read through the MUTABLE `scard`/`sismember`, which are `get_promoted` and
flatten the listpack on the first call -- so from its second assertion on it
was testing a hashtable. It now reads through the `_readonly` twins, checks
the VALUE (SMEMBERS, an integer-encoded member answering to its decimal
spelling) and not just the encoding, and re-asserts `listpack` after every
read. Both threshold tests now assert the AT-threshold half too (128 members,
a 64-byte member), which the pre-fix binary answers `hashtable` to, so
neither can pass vacuously.
Ceiling, stated rather than sold (moon#832): `get_set` is `get_promoted`,
which upgrades unconditionally, and nothing downgrades. Any set command on
the mutable dispatch path -- SREM, SPOP, SMOVE, the store commands, every
command inside MULTI/EXEC or Lua, and the reads whenever they take that path
-- flattens the listpack on first touch. This is a write-only-workload win.
An intset that receives a string member still promotes straight to
`hashtable` (Redis 7.2+ makes it a listpack); named residual of moon#787.
Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh
server per row, 200k keys x `SADD s:i alpha beta gamma`, RSS delta per key,
2 reps): 562 -> 264 B/key (-53%). Redis 7.4.2 measures 106 B/key on the same
probe; the remaining gap is moon's per-key envelope (CompactValue slot +
Box<RedisValue> + the listpack Vec header), not the encoding. The author's
original figure (GCE c3-standard-8: 978.2 -> 404.5 B/key) was a different
probe and is superseded by this one.
Refs moon#787, moon#832. Committed on top of fix/restart-preserves-compact-encoding
(#794 @14b0db85) and lands after it -- without it every set listpack a restart touches is flattened and the
win is a fresh-server artifact; with #794 and without this, a small string
set is `hashtable` live and `listpack` after reload.
author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s it `ZADD` reported `skiplist` from its first member where Redis keeps a zset in a listpack up to zset-max-listpack-entries (128) / zset-max-listpack-value (64). `SortedSetListpack` was wired end to end -- the value codec, both RDB writers, the AOF rewriter, DEBUG DIGEST, MEMORY USAGE, and the read-only `SortedSetRef::Listpack` arm all handled it -- but no accessor ever produced one, so every zset paid the full B+tree-plus-HashMap cost (the 20.5x deficit in moon#787). ZADD now routes through `get_or_create_zset_listpack` below both thresholds and promotes past either; `SortedSetKind::upgrade` gains the listpack arm that keeps every other zset command correct on a key ZADD created compact. Rework of the original branch, stacked on perf/set-listpack-encoding (the SADD rework) on top of fix/restart-preserves-compact-encoding @14b0db85 (itself on main @6251429f), so the three land in that order without a conflict. The blocking finding of the adversarial review is fixed structurally, not patched: - The branch inserted its listpack path ABOVE the moon#814/#820 validation pre-pass, created the key, then parsed scores inside the mutation loop with `return e` on failure -- skipping the charge. `ZADD z 1 a 2 b notafloat c` left `a` and `b` written, uncharged, behind an error reply: the exact regression f2fe28a had merged three days earlier, on the new path. The branch now sits BELOW the pre-pass and above `get_or_create_sorted_set`, so every pair is proven parseable before the keyspace is touched and an erroring ZADD creates no key and writes no prefix. `zadd_that_errors_mid_command_on_the_listpack_path_keeps_the_ledger_exact` and `zadd_listpack_path_is_all_or_nothing_on_a_bad_score` pin both halves; both fail against the submitted shape. - moon#810 bills the arena and members table from real capacity; the promotion now delegates to `SortedSetKind::upgrade` and applies its delta, so the swing has one implementation and one cost model. `zadd_listpack_path_keeps_the_ledger_exact_through_promotion` walks create / in-place update (longer and shorter rendering) / promote / delete. - The member lookup was `iter_pairs()` + `as_bytes()`: two allocations per pair walked, under the hot-path ban (moon#801). It is now a borrowed `iter_pair_refs` scan; the score is decoded from the `ListpackRef` without materialising it. - Scores are rendered into a stack `ScoreBuf` (`storage::zset_score`), not a heap `Bytes` per pair. `render_score` is byte-identical to the command layer's `format_score_bytes` and round-trip exact through `parse_score`; both properties are pinned by tests. The module lives in storage because the decode side renders too. Restart. #794 deliberately excluded zsets from `compact_after_decode` and pinned the exclusion with a tripwire, because a reloaded listpack would have answered WRONGTYPE on the mutable path until the upgrade arm existed. This change is that arm, so it lifts the tripwire in the same commit: the zset arm re-derives a listpack from the B+tree in score order, the tripwire becomes `small_zset_round_trips_back_to_listpack_with_scores_intact` (asserting every member AND every score, including -1.5 and 0.1+0.2), two negative controls keep the past-threshold and oversized cases `skiplist`, and the live restart gate gains a zset key plus a post-restart ZSCORE of its non-integral score. Ceiling, stated rather than sold (moon#832): ZADD is the only zset command that mutates a listpack in place. ZREM, ZINCRBY, ZPOPMIN/ZPOPMAX, the store commands, every zset command inside MULTI/EXEC or Lua, and the reads not in `dispatch_read` at all (ZRANGEBYLEX, ZREVRANGEBYLEX, ZRANDMEMBER, ZINTERCARD) reach the value through `get_promoted` and flatten it on first touch. This is a write-only-workload win. `ZADD z XX 1 a` on a missing key still leaves an empty zset (the moon#830 class, one more site). The CH count keeps main's absolute-EPSILON comparison, now in two places. Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh server per row, 200k keys x `ZADD z:i 1 alpha 2.5 beta 3 gamma`, RSS delta per key, 2 reps): 4165 -> 295 B/key (-93%, 14x). Redis 7.4.2 measures 108 B/key on the same probe; the remaining gap is moon's per-key envelope, not the encoding. Refs moon#787, moon#832, moon#830. Must land AFTER #794 (it edits #794's test and decode arm) and is committed on top of #791; it cannot land before either. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s stop being hashtables
`SetListpack` existed as a storage encoding and `SADD` already had an intset
path, but nothing ever CREATED a string-set listpack: a small set of
non-integer members went straight to `hashtable`, where Redis keeps one in a
listpack up to set-max-listpack-entries (128) / set-max-listpack-value (64).
The guard does not live in `OwnedKind::upgrade` (every impl is unconditional)
-- it lives one level up, in the per-type accessor the command layer calls
INSTEAD of the owned accessor, and the set one was missing:
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_set_listpack MISSING
Rework of the original branch against main @6251429f, which had moved under
it in three ways the adversarial review named:
- moon#803 made `SetValue = IndexSet<Bytes>` and deleted
`Listpack::to_hash_set()`; the branch returned `&mut HashSet<Bytes>` and
could not compile. `upgrade_set_listpack_to_set` now returns `&mut SetValue`
and delegates to `SetKind::upgrade` -- the same conversion `get_or_create`
and `get_promoted` run -- applying the returned delta to `used_memory`
itself, so the listpack -> IndexSet swing has one implementation.
- moon#810 bills the IndexSet from its real capacity (`set_table_bytes`); the
branch's promotion charged only per-member bytes and would have
under-counted the whole table. Delegating to `SetKind::upgrade` makes that
impossible by construction; `sadd_listpack_path_keeps_the_ledger_exact_
through_promotion` walks create / duplicate / promote / delete against a
full recompute.
- moon#801 removed the per-entry allocation from every listpack lookup; the
branch's `lp.iter().any(|m| m.as_bytes() == member)` put it back, one Vec
per entry walked, in a file under the hot-path allocation ban. The scan is
`Listpack::contains_element` -- the borrowed comparison #801 added for
exactly this lookup. A non-bulk argument is skipped, as the standard path
does, never returned from inside the charge window (moon#814 shape).
The misleading test is corrected. `listpack_set_answers_reads_identically`
read through the MUTABLE `scard`/`sismember`, which are `get_promoted` and
flatten the listpack on the first call -- so from its second assertion on it
was testing a hashtable. It now reads through the `_readonly` twins, checks
the VALUE (SMEMBERS, an integer-encoded member answering to its decimal
spelling) and not just the encoding, and re-asserts `listpack` after every
read. Both threshold tests now assert the AT-threshold half too (128 members,
a 64-byte member), which the pre-fix binary answers `hashtable` to, so
neither can pass vacuously.
Ceiling, stated rather than sold (moon#832): `get_set` is `get_promoted`,
which upgrades unconditionally, and nothing downgrades. Any set command on
the mutable dispatch path -- SREM, SPOP, SMOVE, the store commands, every
command inside MULTI/EXEC or Lua, and the reads whenever they take that path
-- flattens the listpack on first touch. This is a write-only-workload win.
An intset that receives a string member still promotes straight to
`hashtable` (Redis 7.2+ makes it a listpack); named residual of moon#787.
Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh
server per row, 200k keys x `SADD s:i alpha beta gamma`, RSS delta per key,
2 reps): 562 -> 264 B/key (-53%). Redis 7.4.2 measures 106 B/key on the same
probe; the remaining gap is moon's per-key envelope (CompactValue slot +
Box<RedisValue> + the listpack Vec header), not the encoding. The author's
original figure (GCE c3-standard-8: 978.2 -> 404.5 B/key) was a different
probe and is superseded by this one.
Refs moon#787, moon#832. Committed on top of fix/restart-preserves-compact-encoding
(#794 @14b0db85) and lands after it -- without it every set listpack a restart touches is flattened and the
win is a fresh-server artifact; with #794 and without this, a small string
set is `hashtable` live and `listpack` after reload.
author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s stop being hashtables
`SetListpack` existed as a storage encoding and `SADD` already had an intset
path, but nothing ever CREATED a string-set listpack: a small set of
non-integer members went straight to `hashtable`, where Redis keeps one in a
listpack up to set-max-listpack-entries (128) / set-max-listpack-value (64).
The guard does not live in `OwnedKind::upgrade` (every impl is unconditional)
-- it lives one level up, in the per-type accessor the command layer calls
INSTEAD of the owned accessor, and the set one was missing:
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_set_listpack MISSING
Rework of the original branch against main @6251429f, which had moved under
it in three ways the adversarial review named:
- moon#803 made `SetValue = IndexSet<Bytes>` and deleted
`Listpack::to_hash_set()`; the branch returned `&mut HashSet<Bytes>` and
could not compile. `upgrade_set_listpack_to_set` now returns `&mut SetValue`
and delegates to `SetKind::upgrade` -- the same conversion `get_or_create`
and `get_promoted` run -- applying the returned delta to `used_memory`
itself, so the listpack -> IndexSet swing has one implementation.
- moon#810 bills the IndexSet from its real capacity (`set_table_bytes`); the
branch's promotion charged only per-member bytes and would have
under-counted the whole table. Delegating to `SetKind::upgrade` makes that
impossible by construction; `sadd_listpack_path_keeps_the_ledger_exact_
through_promotion` walks create / duplicate / promote / delete against a
full recompute.
- moon#801 removed the per-entry allocation from every listpack lookup; the
branch's `lp.iter().any(|m| m.as_bytes() == member)` put it back, one Vec
per entry walked, in a file under the hot-path allocation ban. The scan is
`Listpack::contains_element` -- the borrowed comparison #801 added for
exactly this lookup. A non-bulk argument is skipped, as the standard path
does, never returned from inside the charge window (moon#814 shape).
The misleading test is corrected. `listpack_set_answers_reads_identically`
read through the MUTABLE `scard`/`sismember`, which are `get_promoted` and
flatten the listpack on the first call -- so from its second assertion on it
was testing a hashtable. It now reads through the `_readonly` twins, checks
the VALUE (SMEMBERS, an integer-encoded member answering to its decimal
spelling) and not just the encoding, and re-asserts `listpack` after every
read. Both threshold tests now assert the AT-threshold half too (128 members,
a 64-byte member), which the pre-fix binary answers `hashtable` to, so
neither can pass vacuously.
Ceiling, stated rather than sold (moon#832): `get_set` is `get_promoted`,
which upgrades unconditionally, and nothing downgrades. Any set command on
the mutable dispatch path -- SREM, SPOP, SMOVE, the store commands, every
command inside MULTI/EXEC or Lua, and the reads whenever they take that path
-- flattens the listpack on first touch. This is a write-only-workload win.
An intset that receives a string member still promotes straight to
`hashtable` (Redis 7.2+ makes it a listpack); named residual of moon#787.
Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh
server per row, 200k keys x `SADD s:i alpha beta gamma`, RSS delta per key,
2 reps): 562 -> 264 B/key (-53%). Redis 7.4.2 measures 106 B/key on the same
probe; the remaining gap is moon's per-key envelope (CompactValue slot +
Box<RedisValue> + the listpack Vec header), not the encoding. The author's
original figure (GCE c3-standard-8: 978.2 -> 404.5 B/key) was a different
probe and is superseded by this one.
Refs moon#787, moon#832. Committed on top of fix/restart-preserves-compact-encoding
(#794 @14b0db85) and lands after it -- without it every set listpack a restart touches is flattened and the
win is a fresh-server artifact; with #794 and without this, a small string
set is `hashtable` live and `listpack` after reload.
author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s it `ZADD` reported `skiplist` from its first member where Redis keeps a zset in a listpack up to zset-max-listpack-entries (128) / zset-max-listpack-value (64). `SortedSetListpack` was wired end to end -- the value codec, both RDB writers, the AOF rewriter, DEBUG DIGEST, MEMORY USAGE, and the read-only `SortedSetRef::Listpack` arm all handled it -- but no accessor ever produced one, so every zset paid the full B+tree-plus-HashMap cost (the 20.5x deficit in moon#787). ZADD now routes through `get_or_create_zset_listpack` below both thresholds and promotes past either; `SortedSetKind::upgrade` gains the listpack arm that keeps every other zset command correct on a key ZADD created compact. Rework of the original branch, stacked on perf/set-listpack-encoding (the SADD rework) on top of fix/restart-preserves-compact-encoding @14b0db85 (itself on main @6251429f), so the three land in that order without a conflict. The blocking finding of the adversarial review is fixed structurally, not patched: - The branch inserted its listpack path ABOVE the moon#814/#820 validation pre-pass, created the key, then parsed scores inside the mutation loop with `return e` on failure -- skipping the charge. `ZADD z 1 a 2 b notafloat c` left `a` and `b` written, uncharged, behind an error reply: the exact regression f2fe28a had merged three days earlier, on the new path. The branch now sits BELOW the pre-pass and above `get_or_create_sorted_set`, so every pair is proven parseable before the keyspace is touched and an erroring ZADD creates no key and writes no prefix. `zadd_that_errors_mid_command_on_the_listpack_path_keeps_the_ledger_exact` and `zadd_listpack_path_is_all_or_nothing_on_a_bad_score` pin both halves; both fail against the submitted shape. - moon#810 bills the arena and members table from real capacity; the promotion now delegates to `SortedSetKind::upgrade` and applies its delta, so the swing has one implementation and one cost model. `zadd_listpack_path_keeps_the_ledger_exact_through_promotion` walks create / in-place update (longer and shorter rendering) / promote / delete. - The member lookup was `iter_pairs()` + `as_bytes()`: two allocations per pair walked, under the hot-path ban (moon#801). It is now a borrowed `iter_pair_refs` scan; the score is decoded from the `ListpackRef` without materialising it. - Scores are rendered into a stack `ScoreBuf` (`storage::zset_score`), not a heap `Bytes` per pair. `render_score` is byte-identical to the command layer's `format_score_bytes` and round-trip exact through `parse_score`; both properties are pinned by tests. The module lives in storage because the decode side renders too. Restart. #794 deliberately excluded zsets from `compact_after_decode` and pinned the exclusion with a tripwire, because a reloaded listpack would have answered WRONGTYPE on the mutable path until the upgrade arm existed. This change is that arm, so it lifts the tripwire in the same commit: the zset arm re-derives a listpack from the B+tree in score order, the tripwire becomes `small_zset_round_trips_back_to_listpack_with_scores_intact` (asserting every member AND every score, including -1.5 and 0.1+0.2), two negative controls keep the past-threshold and oversized cases `skiplist`, and the live restart gate gains a zset key plus a post-restart ZSCORE of its non-integral score. Ceiling, stated rather than sold (moon#832): ZADD is the only zset command that mutates a listpack in place. ZREM, ZINCRBY, ZPOPMIN/ZPOPMAX, the store commands, every zset command inside MULTI/EXEC or Lua, and the reads not in `dispatch_read` at all (ZRANGEBYLEX, ZREVRANGEBYLEX, ZRANDMEMBER, ZINTERCARD) reach the value through `get_promoted` and flatten it on first touch. This is a write-only-workload win. `ZADD z XX 1 a` on a missing key still leaves an empty zset (the moon#830 class, one more site). The CH count keeps main's absolute-EPSILON comparison, now in two places. Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh server per row, 200k keys x `ZADD z:i 1 alpha 2.5 beta 3 gamma`, RSS delta per key, 2 reps): 4165 -> 295 B/key (-93%, 14x). Redis 7.4.2 measures 108 B/key on the same probe; the remaining gap is moon's per-key envelope, not the encoding. Refs moon#787, moon#832, moon#830. Must land AFTER #794 (it edits #794's test and decode arm) and is committed on top of #791; it cannot land before either. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s stop being hashtables
`SetListpack` existed as a storage encoding and `SADD` already had an intset
path, but nothing ever CREATED a string-set listpack: a small set of
non-integer members went straight to `hashtable`, where Redis keeps one in a
listpack up to set-max-listpack-entries (128) / set-max-listpack-value (64).
The guard does not live in `OwnedKind::upgrade` (every impl is unconditional)
-- it lives one level up, in the per-type accessor the command layer calls
INSTEAD of the owned accessor, and the set one was missing:
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_set_listpack MISSING
Rework of the original branch against main @6251429f, which had moved under
it in three ways the adversarial review named:
- moon#803 made `SetValue = IndexSet<Bytes>` and deleted
`Listpack::to_hash_set()`; the branch returned `&mut HashSet<Bytes>` and
could not compile. `upgrade_set_listpack_to_set` now returns `&mut SetValue`
and delegates to `SetKind::upgrade` -- the same conversion `get_or_create`
and `get_promoted` run -- applying the returned delta to `used_memory`
itself, so the listpack -> IndexSet swing has one implementation.
- moon#810 bills the IndexSet from its real capacity (`set_table_bytes`); the
branch's promotion charged only per-member bytes and would have
under-counted the whole table. Delegating to `SetKind::upgrade` makes that
impossible by construction; `sadd_listpack_path_keeps_the_ledger_exact_
through_promotion` walks create / duplicate / promote / delete against a
full recompute.
- moon#801 removed the per-entry allocation from every listpack lookup; the
branch's `lp.iter().any(|m| m.as_bytes() == member)` put it back, one Vec
per entry walked, in a file under the hot-path allocation ban. The scan is
`Listpack::contains_element` -- the borrowed comparison #801 added for
exactly this lookup. A non-bulk argument is skipped, as the standard path
does, never returned from inside the charge window (moon#814 shape).
The misleading test is corrected. `listpack_set_answers_reads_identically`
read through the MUTABLE `scard`/`sismember`, which are `get_promoted` and
flatten the listpack on the first call -- so from its second assertion on it
was testing a hashtable. It now reads through the `_readonly` twins, checks
the VALUE (SMEMBERS, an integer-encoded member answering to its decimal
spelling) and not just the encoding, and re-asserts `listpack` after every
read. Both threshold tests now assert the AT-threshold half too (128 members,
a 64-byte member), which the pre-fix binary answers `hashtable` to, so
neither can pass vacuously.
Ceiling, stated rather than sold (moon#832): `get_set` is `get_promoted`,
which upgrades unconditionally, and nothing downgrades. Any set command on
the mutable dispatch path -- SREM, SPOP, SMOVE, the store commands, every
command inside MULTI/EXEC or Lua, and the reads whenever they take that path
-- flattens the listpack on first touch. This is a write-only-workload win.
An intset that receives a string member still promotes straight to
`hashtable` (Redis 7.2+ makes it a listpack); named residual of moon#787.
Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh
server per row, 200k keys x `SADD s:i alpha beta gamma`, RSS delta per key,
2 reps): 562 -> 264 B/key (-53%). Redis 7.4.2 measures 106 B/key on the same
probe; the remaining gap is moon's per-key envelope (CompactValue slot +
Box<RedisValue> + the listpack Vec header), not the encoding. The author's
original figure (GCE c3-standard-8: 978.2 -> 404.5 B/key) was a different
probe and is superseded by this one.
Refs moon#787, moon#832. Committed on top of fix/restart-preserves-compact-encoding
(#794 @14b0db85) and lands after it -- without it every set listpack a restart touches is flattened and the
win is a fresh-server artifact; with #794 and without this, a small string
set is `hashtable` live and `listpack` after reload.
author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s stop being hashtables
`SetListpack` existed as a storage encoding and `SADD` already had an intset
path, but nothing ever CREATED a string-set listpack: a small set of
non-integer members went straight to `hashtable`, where Redis keeps one in a
listpack up to set-max-listpack-entries (128) / set-max-listpack-value (64).
The guard does not live in `OwnedKind::upgrade` (every impl is unconditional)
-- it lives one level up, in the per-type accessor the command layer calls
INSTEAD of the owned accessor, and the set one was missing:
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_set_listpack MISSING
Rework of the original branch against main @6251429f, which had moved under
it in three ways the adversarial review named:
- moon#803 made `SetValue = IndexSet<Bytes>` and deleted
`Listpack::to_hash_set()`; the branch returned `&mut HashSet<Bytes>` and
could not compile. `upgrade_set_listpack_to_set` now returns `&mut SetValue`
and delegates to `SetKind::upgrade` -- the same conversion `get_or_create`
and `get_promoted` run -- applying the returned delta to `used_memory`
itself, so the listpack -> IndexSet swing has one implementation.
- moon#810 bills the IndexSet from its real capacity (`set_table_bytes`); the
branch's promotion charged only per-member bytes and would have
under-counted the whole table. Delegating to `SetKind::upgrade` makes that
impossible by construction; `sadd_listpack_path_keeps_the_ledger_exact_
through_promotion` walks create / duplicate / promote / delete against a
full recompute.
- moon#801 removed the per-entry allocation from every listpack lookup; the
branch's `lp.iter().any(|m| m.as_bytes() == member)` put it back, one Vec
per entry walked, in a file under the hot-path allocation ban. The scan is
`Listpack::contains_element` -- the borrowed comparison #801 added for
exactly this lookup. A non-bulk argument is skipped, as the standard path
does, never returned from inside the charge window (moon#814 shape).
The misleading test is corrected. `listpack_set_answers_reads_identically`
read through the MUTABLE `scard`/`sismember`, which are `get_promoted` and
flatten the listpack on the first call -- so from its second assertion on it
was testing a hashtable. It now reads through the `_readonly` twins, checks
the VALUE (SMEMBERS, an integer-encoded member answering to its decimal
spelling) and not just the encoding, and re-asserts `listpack` after every
read. Both threshold tests now assert the AT-threshold half too (128 members,
a 64-byte member), which the pre-fix binary answers `hashtable` to, so
neither can pass vacuously.
Ceiling, stated rather than sold (moon#832): `get_set` is `get_promoted`,
which upgrades unconditionally, and nothing downgrades. Any set command on
the mutable dispatch path -- SREM, SPOP, SMOVE, the store commands, every
command inside MULTI/EXEC or Lua, and the reads whenever they take that path
-- flattens the listpack on first touch. This is a write-only-workload win.
An intset that receives a string member still promotes straight to
`hashtable` (Redis 7.2+ makes it a listpack); named residual of moon#787.
Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh
server per row, 200k keys x `SADD s:i alpha beta gamma`, RSS delta per key,
2 reps): 562 -> 264 B/key (-53%). Redis 7.4.2 measures 106 B/key on the same
probe; the remaining gap is moon's per-key envelope (CompactValue slot +
Box<RedisValue> + the listpack Vec header), not the encoding. The author's
original figure (GCE c3-standard-8: 978.2 -> 404.5 B/key) was a different
probe and is superseded by this one.
Refs moon#787, moon#832. Committed on top of fix/restart-preserves-compact-encoding
(#794 @14b0db85) and lands after it -- without it every set listpack a restart touches is flattened and the
win is a fresh-server artifact; with #794 and without this, a small string
set is `hashtable` live and `listpack` after reload.
author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s it `ZADD` reported `skiplist` from its first member where Redis keeps a zset in a listpack up to zset-max-listpack-entries (128) / zset-max-listpack-value (64). `SortedSetListpack` was wired end to end -- the value codec, both RDB writers, the AOF rewriter, DEBUG DIGEST, MEMORY USAGE, and the read-only `SortedSetRef::Listpack` arm all handled it -- but no accessor ever produced one, so every zset paid the full B+tree-plus-HashMap cost (the 20.5x deficit in moon#787). ZADD now routes through `get_or_create_zset_listpack` below both thresholds and promotes past either; `SortedSetKind::upgrade` gains the listpack arm that keeps every other zset command correct on a key ZADD created compact. Rework of the original branch, stacked on perf/set-listpack-encoding (the SADD rework) on top of fix/restart-preserves-compact-encoding @14b0db85 (itself on main @6251429f), so the three land in that order without a conflict. The blocking finding of the adversarial review is fixed structurally, not patched: - The branch inserted its listpack path ABOVE the moon#814/#820 validation pre-pass, created the key, then parsed scores inside the mutation loop with `return e` on failure -- skipping the charge. `ZADD z 1 a 2 b notafloat c` left `a` and `b` written, uncharged, behind an error reply: the exact regression f2fe28a had merged three days earlier, on the new path. The branch now sits BELOW the pre-pass and above `get_or_create_sorted_set`, so every pair is proven parseable before the keyspace is touched and an erroring ZADD creates no key and writes no prefix. `zadd_that_errors_mid_command_on_the_listpack_path_keeps_the_ledger_exact` and `zadd_listpack_path_is_all_or_nothing_on_a_bad_score` pin both halves; both fail against the submitted shape. - moon#810 bills the arena and members table from real capacity; the promotion now delegates to `SortedSetKind::upgrade` and applies its delta, so the swing has one implementation and one cost model. `zadd_listpack_path_keeps_the_ledger_exact_through_promotion` walks create / in-place update (longer and shorter rendering) / promote / delete. - The member lookup was `iter_pairs()` + `as_bytes()`: two allocations per pair walked, under the hot-path ban (moon#801). It is now a borrowed `iter_pair_refs` scan; the score is decoded from the `ListpackRef` without materialising it. - Scores are rendered into a stack `ScoreBuf` (`storage::zset_score`), not a heap `Bytes` per pair. `render_score` is byte-identical to the command layer's `format_score_bytes` and round-trip exact through `parse_score`; both properties are pinned by tests. The module lives in storage because the decode side renders too. Restart. #794 deliberately excluded zsets from `compact_after_decode` and pinned the exclusion with a tripwire, because a reloaded listpack would have answered WRONGTYPE on the mutable path until the upgrade arm existed. This change is that arm, so it lifts the tripwire in the same commit: the zset arm re-derives a listpack from the B+tree in score order, the tripwire becomes `small_zset_round_trips_back_to_listpack_with_scores_intact` (asserting every member AND every score, including -1.5 and 0.1+0.2), two negative controls keep the past-threshold and oversized cases `skiplist`, and the live restart gate gains a zset key plus a post-restart ZSCORE of its non-integral score. Ceiling, stated rather than sold (moon#832): ZADD is the only zset command that mutates a listpack in place. ZREM, ZINCRBY, ZPOPMIN/ZPOPMAX, the store commands, every zset command inside MULTI/EXEC or Lua, and the reads not in `dispatch_read` at all (ZRANGEBYLEX, ZREVRANGEBYLEX, ZRANDMEMBER, ZINTERCARD) reach the value through `get_promoted` and flatten it on first touch. This is a write-only-workload win. `ZADD z XX 1 a` on a missing key still leaves an empty zset (the moon#830 class, one more site). The CH count keeps main's absolute-EPSILON comparison, now in two places. Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh server per row, 200k keys x `ZADD z:i 1 alpha 2.5 beta 3 gamma`, RSS delta per key, 2 reps): 4165 -> 295 B/key (-93%, 14x). Redis 7.4.2 measures 108 B/key on the same probe; the remaining gap is moon's per-key envelope, not the encoding. Refs moon#787, moon#832, moon#830. Must land AFTER #794 (it edits #794's test and decode arm) and is committed on top of #791; it cannot land before either. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
… stop being hashtables (#877) * perf(storage): SADD reaches its listpack encoding -- small string sets stop being hashtables `SetListpack` existed as a storage encoding and `SADD` already had an intset path, but nothing ever CREATED a string-set listpack: a small set of non-integer members went straight to `hashtable`, where Redis keeps one in a listpack up to set-max-listpack-entries (128) / set-max-listpack-value (64). The guard does not live in `OwnedKind::upgrade` (every impl is unconditional) -- it lives one level up, in the per-type accessor the command layer calls INSTEAD of the owned accessor, and the set one was missing: 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_set_listpack MISSING Rework of the original branch against main @6251429f, which had moved under it in three ways the adversarial review named: - moon#803 made `SetValue = IndexSet<Bytes>` and deleted `Listpack::to_hash_set()`; the branch returned `&mut HashSet<Bytes>` and could not compile. `upgrade_set_listpack_to_set` now returns `&mut SetValue` and delegates to `SetKind::upgrade` -- the same conversion `get_or_create` and `get_promoted` run -- applying the returned delta to `used_memory` itself, so the listpack -> IndexSet swing has one implementation. - moon#810 bills the IndexSet from its real capacity (`set_table_bytes`); the branch's promotion charged only per-member bytes and would have under-counted the whole table. Delegating to `SetKind::upgrade` makes that impossible by construction; `sadd_listpack_path_keeps_the_ledger_exact_ through_promotion` walks create / duplicate / promote / delete against a full recompute. - moon#801 removed the per-entry allocation from every listpack lookup; the branch's `lp.iter().any(|m| m.as_bytes() == member)` put it back, one Vec per entry walked, in a file under the hot-path allocation ban. The scan is `Listpack::contains_element` -- the borrowed comparison #801 added for exactly this lookup. A non-bulk argument is skipped, as the standard path does, never returned from inside the charge window (moon#814 shape). The misleading test is corrected. `listpack_set_answers_reads_identically` read through the MUTABLE `scard`/`sismember`, which are `get_promoted` and flatten the listpack on the first call -- so from its second assertion on it was testing a hashtable. It now reads through the `_readonly` twins, checks the VALUE (SMEMBERS, an integer-encoded member answering to its decimal spelling) and not just the encoding, and re-asserts `listpack` after every read. Both threshold tests now assert the AT-threshold half too (128 members, a 64-byte member), which the pre-fix binary answers `hashtable` to, so neither can pass vacuously. Ceiling, stated rather than sold (moon#832): `get_set` is `get_promoted`, which upgrades unconditionally, and nothing downgrades. Any set command on the mutable dispatch path -- SREM, SPOP, SMOVE, the store commands, every command inside MULTI/EXEC or Lua, and the reads whenever they take that path -- flattens the listpack on first touch. This is a write-only-workload win. An intset that receives a string member still promotes straight to `hashtable` (Redis 7.2+ makes it a listpack); named residual of moon#787. Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh server per row, 200k keys x `SADD s:i alpha beta gamma`, RSS delta per key, 2 reps): 562 -> 264 B/key (-53%). Redis 7.4.2 measures 106 B/key on the same probe; the remaining gap is moon's per-key envelope (CompactValue slot + Box<RedisValue> + the listpack Vec header), not the encoding. The author's original figure (GCE c3-standard-8: 978.2 -> 404.5 B/key) was a different probe and is superseded by this one. Refs moon#787, moon#832. Committed on top of fix/restart-preserves-compact-encoding (#794 @14b0db85) and lands after it -- without it every set listpack a restart touches is flattened and the win is a fresh-server artifact; with #794 and without this, a small string set is `hashtable` live and `listpack` after reload. author: Tin Dang * fix(storage): bound SADD's listpack batch so a large call stops losing 65,536 members This branch adds a listpack path to SADD, and inherited the moon#865 shape with it: every member is pushed into the listpack and only then is the entry count compared with LISTPACK_MAX_ENTRIES. The header counts elements in a u16, so a single large SADD wrapped it. Measured on this branch before the guard, against `Database` directly: SADD big m0000000 .. m0069999 (70,000 members) SADD replied Integer(70000) SCARD replied Integer(4464) <- 70000 - 65536 The write is acknowledged and 65,536 members become unreachable. It also made the call quadratic: `contains_element` is a linear in-place scan, so an unbounded batch is ~2.4e9 comparisons inside one command on one shard thread (7.7s measured; 0.01s after). moon#866 fixed the same shape for RPUSH/LPUSH/HSET on main and added `listpack_batch_fits`. This applies it here, which is the whole change: a call carrying more than LISTPACK_MAX_ENTRIES new members skips the listpack encoding, exactly as an oversized member already did. That container was going to be upgraded on the next line regardless. Two tests, both run against the unguarded tree first: without the guard the overflow test reports 65535 (the saturating backstop moon#866 added to `update_header`), and 4464 with neither guard. The second test pins that the guard does not simply disable the encoding -- n = 1, 8, 128, 129 still land in a listpack, and 700 calls of 100 members still accumulate to 70,000. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…s it `ZADD` reported `skiplist` from its first member where Redis keeps a zset in a listpack up to zset-max-listpack-entries (128) / zset-max-listpack-value (64). `SortedSetListpack` was wired end to end -- the value codec, both RDB writers, the AOF rewriter, DEBUG DIGEST, MEMORY USAGE, and the read-only `SortedSetRef::Listpack` arm all handled it -- but no accessor ever produced one, so every zset paid the full B+tree-plus-HashMap cost (the 20.5x deficit in moon#787). ZADD now routes through `get_or_create_zset_listpack` below both thresholds and promotes past either; `SortedSetKind::upgrade` gains the listpack arm that keeps every other zset command correct on a key ZADD created compact. Rework of the original branch, stacked on perf/set-listpack-encoding (the SADD rework) on top of fix/restart-preserves-compact-encoding @14b0db85 (itself on main @6251429f), so the three land in that order without a conflict. The blocking finding of the adversarial review is fixed structurally, not patched: - The branch inserted its listpack path ABOVE the moon#814/#820 validation pre-pass, created the key, then parsed scores inside the mutation loop with `return e` on failure -- skipping the charge. `ZADD z 1 a 2 b notafloat c` left `a` and `b` written, uncharged, behind an error reply: the exact regression f2fe28a had merged three days earlier, on the new path. The branch now sits BELOW the pre-pass and above `get_or_create_sorted_set`, so every pair is proven parseable before the keyspace is touched and an erroring ZADD creates no key and writes no prefix. `zadd_that_errors_mid_command_on_the_listpack_path_keeps_the_ledger_exact` and `zadd_listpack_path_is_all_or_nothing_on_a_bad_score` pin both halves; both fail against the submitted shape. - moon#810 bills the arena and members table from real capacity; the promotion now delegates to `SortedSetKind::upgrade` and applies its delta, so the swing has one implementation and one cost model. `zadd_listpack_path_keeps_the_ledger_exact_through_promotion` walks create / in-place update (longer and shorter rendering) / promote / delete. - The member lookup was `iter_pairs()` + `as_bytes()`: two allocations per pair walked, under the hot-path ban (moon#801). It is now a borrowed `iter_pair_refs` scan; the score is decoded from the `ListpackRef` without materialising it. - Scores are rendered into a stack `ScoreBuf` (`storage::zset_score`), not a heap `Bytes` per pair. `render_score` is byte-identical to the command layer's `format_score_bytes` and round-trip exact through `parse_score`; both properties are pinned by tests. The module lives in storage because the decode side renders too. Restart. #794 deliberately excluded zsets from `compact_after_decode` and pinned the exclusion with a tripwire, because a reloaded listpack would have answered WRONGTYPE on the mutable path until the upgrade arm existed. This change is that arm, so it lifts the tripwire in the same commit: the zset arm re-derives a listpack from the B+tree in score order, the tripwire becomes `small_zset_round_trips_back_to_listpack_with_scores_intact` (asserting every member AND every score, including -1.5 and 0.1+0.2), two negative controls keep the past-threshold and oversized cases `skiplist`, and the live restart gate gains a zset key plus a post-restart ZSCORE of its non-integral score. Ceiling, stated rather than sold (moon#832): ZADD is the only zset command that mutates a listpack in place. ZREM, ZINCRBY, ZPOPMIN/ZPOPMAX, the store commands, every zset command inside MULTI/EXEC or Lua, and the reads not in `dispatch_read` at all (ZRANGEBYLEX, ZREVRANGEBYLEX, ZRANDMEMBER, ZINTERCARD) reach the value through `get_promoted` and flatten it on first touch. This is a write-only-workload win. `ZADD z XX 1 a` on a missing key still leaves an empty zset (the moon#830 class, one more site). The CH count keeps main's absolute-EPSILON comparison, now in two places. Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh server per row, 200k keys x `ZADD z:i 1 alpha 2.5 beta 3 gamma`, RSS delta per key, 2 reps): 4165 -> 295 B/key (-93%, 14x). Redis 7.4.2 measures 108 B/key on the same probe; the remaining gap is moon's per-key envelope, not the encoding. Refs moon#787, moon#832, moon#830. Must land AFTER #794 (it edits #794's test and decode arm) and is committed on top of #791; it cannot land before either. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Sep 8, 2026
…being skiplists (#878) * perf(storage): ZADD reaches its listpack encoding, and a restart keeps it `ZADD` reported `skiplist` from its first member where Redis keeps a zset in a listpack up to zset-max-listpack-entries (128) / zset-max-listpack-value (64). `SortedSetListpack` was wired end to end -- the value codec, both RDB writers, the AOF rewriter, DEBUG DIGEST, MEMORY USAGE, and the read-only `SortedSetRef::Listpack` arm all handled it -- but no accessor ever produced one, so every zset paid the full B+tree-plus-HashMap cost (the 20.5x deficit in moon#787). ZADD now routes through `get_or_create_zset_listpack` below both thresholds and promotes past either; `SortedSetKind::upgrade` gains the listpack arm that keeps every other zset command correct on a key ZADD created compact. Rework of the original branch, stacked on perf/set-listpack-encoding (the SADD rework) on top of fix/restart-preserves-compact-encoding @14b0db85 (itself on main @6251429f), so the three land in that order without a conflict. The blocking finding of the adversarial review is fixed structurally, not patched: - The branch inserted its listpack path ABOVE the moon#814/#820 validation pre-pass, created the key, then parsed scores inside the mutation loop with `return e` on failure -- skipping the charge. `ZADD z 1 a 2 b notafloat c` left `a` and `b` written, uncharged, behind an error reply: the exact regression f2fe28a had merged three days earlier, on the new path. The branch now sits BELOW the pre-pass and above `get_or_create_sorted_set`, so every pair is proven parseable before the keyspace is touched and an erroring ZADD creates no key and writes no prefix. `zadd_that_errors_mid_command_on_the_listpack_path_keeps_the_ledger_exact` and `zadd_listpack_path_is_all_or_nothing_on_a_bad_score` pin both halves; both fail against the submitted shape. - moon#810 bills the arena and members table from real capacity; the promotion now delegates to `SortedSetKind::upgrade` and applies its delta, so the swing has one implementation and one cost model. `zadd_listpack_path_keeps_the_ledger_exact_through_promotion` walks create / in-place update (longer and shorter rendering) / promote / delete. - The member lookup was `iter_pairs()` + `as_bytes()`: two allocations per pair walked, under the hot-path ban (moon#801). It is now a borrowed `iter_pair_refs` scan; the score is decoded from the `ListpackRef` without materialising it. - Scores are rendered into a stack `ScoreBuf` (`storage::zset_score`), not a heap `Bytes` per pair. `render_score` is byte-identical to the command layer's `format_score_bytes` and round-trip exact through `parse_score`; both properties are pinned by tests. The module lives in storage because the decode side renders too. Restart. #794 deliberately excluded zsets from `compact_after_decode` and pinned the exclusion with a tripwire, because a reloaded listpack would have answered WRONGTYPE on the mutable path until the upgrade arm existed. This change is that arm, so it lifts the tripwire in the same commit: the zset arm re-derives a listpack from the B+tree in score order, the tripwire becomes `small_zset_round_trips_back_to_listpack_with_scores_intact` (asserting every member AND every score, including -1.5 and 0.1+0.2), two negative controls keep the past-threshold and oversized cases `skiplist`, and the live restart gate gains a zset key plus a post-restart ZSCORE of its non-integral score. Ceiling, stated rather than sold (moon#832): ZADD is the only zset command that mutates a listpack in place. ZREM, ZINCRBY, ZPOPMIN/ZPOPMAX, the store commands, every zset command inside MULTI/EXEC or Lua, and the reads not in `dispatch_read` at all (ZRANGEBYLEX, ZREVRANGEBYLEX, ZRANDMEMBER, ZINTERCARD) reach the value through `get_promoted` and flatten it on first touch. This is a write-only-workload win. `ZADD z XX 1 a` on a missing key still leaves an empty zset (the moon#830 class, one more site). The CH count keeps main's absolute-EPSILON comparison, now in two places. Measured on Linux (moon-bench-x86, x86_64, load < 1.0, --shards 1, fresh server per row, 200k keys x `ZADD z:i 1 alpha 2.5 beta 3 gamma`, RSS delta per key, 2 reps): 4165 -> 295 B/key (-93%, 14x). Redis 7.4.2 measures 108 B/key on the same probe; the remaining gap is moon's per-key envelope, not the encoding. Refs moon#787, moon#832, moon#830. Must land AFTER #794 (it edits #794's test and decode arm) and is committed on top of #791; it cannot land before either. author: Tin Dang * fix(storage): bound ZADD's listpack batch so a large call stops losing members This branch adds a listpack path to ZADD and inherited the moon#865 shape with it: every pair is pushed into the listpack and only then is the entry count compared with LISTPACK_MAX_ENTRIES. The header counts elements in a u16, so a single large ZADD wrapped it. A zset listpack stores TWO entries per member -- member then score -- so the wrap arrives at 32,768 members, half the list/set threshold. Measured on this branch before the guard: ZADD bigz 0 m0000000 .. 39999 m0039999 (40,000 members) ZADD replied Integer(40000) ZCARD replied Integer(32767) moon#866 fixed the same shape for RPUSH/LPUSH/HSET on main and added `listpack_batch_fits`; moon#791 applied it to SADD. This applies it here. The entry count is `remaining.len()`, not the pair count, because of the two entries per member -- passing the pair count would leave the guard off by 2x and still allow a wrap. Two tests, run against the unguarded tree first: the overflow test reports 32767 without the guard. The second pins that the guard does not simply disable the encoding -- n = 1, 8, 64, 65 still land in a listpack, and 400 calls of 100 members still accumulate to 40,000. author: Tin Dang
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
RDB decode rebuilt every container in its full form, so a listpack hash, a listpack
list, an intset and a set listpack all came back flattened on the first reload:
DEBUG RELOADRedis preserves every one of these. moon preserved none.
This is the critical path for the whole memory campaign. The #787 SADD-listpack win
(set 978.2 -> 404.5 B/key on Linux) reverted to 978.2 — 1.85x back to 4.48x vs redis
7.4.2 — the moment the server came back up. Every per-type encoding win is conditional on
this fix, and every RSS figure measured on a never-restarted server is an upper bound, not
a steady state.
The trap: two decoders, and the obvious one is not the one that runs
src/persistence/rdb.rshas two decode paths:value_codec::decode_value_body— the discoverable one.read_entry_zero_copy— a hand-rolled decoder that builds each container inline andnever calls
value_codecat all.#2 is the path a restart actually takes, including
load_from_bytes, the AOF RDBpreamble. My first attempt hooked only #1: everything compiled, the lib suite stayed
green, and the guard test still failed with all four types flattened. Both funnels are
covered now. (Three
CompactValue::from_redis_valuefunnels exist in that file; thestring one needs no compaction.)
Change
value_codec::compact_after_decodere-derives the compact encoding against thresholdsalready in the tree (
LISTPACK_MAX_ENTRIES128,LISTPACK_MAX_ELEMENT_SIZE64,INTSET_MAX_ENTRIES512), applied at both RDB funnels.No wire-format change. Every listpack variant already maps to the same
ValueTypetagas its full form (
value_codec::value_type_of), so files stay readable in bothdirections. They are not byte-identical — a listpack preserves insertion order where a
HashMapdoes not; the type tag and field/value encoding are unchanged.HashWithTtlis deliberately not compacted: a listpack carries no TTL sidecar.Deliberately NOT the cold/spill path
ValueKind::classify_coldaccepts only the canonical full forms, so a cold-decodedSetListpackfalls through its_ => Err(WrongType)arm and would answer WRONGTYPE fora perfectly valid set. The first version of this fix compacted the shared
decode_value_bodyand turned 11 cold-tier tests red for exactly that reason. Compactionis therefore opt-in (
decode_value_body_compacting), RDB-path only.Two named follow-ups: widening
classify_coldso the cold tier compacts too, and theredis_rdb.rsread side used by DUMP/RESTORE and replica full sync — the latter means areplica can currently hold more memory than its master for identical data.
Tests — red/green
tests/restart_preserves_compact_encoding.rswrites one key of each compact type plus a200-field hash as a negative control (without it, a bug that compacted everything
would pass),
BGREWRITEAOFs, restarts a real server on the same--dir, and asserts thetarget encoding of all five.
It asserts the post-restart encoding absolutely rather than
before == after, becausebefore == afteris branch-dependent: a small string set is ahashtableon main and alistpackon #787's branch. Stated absolutely, the test is meaningful on both.Red:
h: was listpack, after restart hashtable / l: ... linkedlist / si: ... hashtable.Green after the fix.
Refs #787
author: Tin Dang