[Topic-2 PR-B] compression-stress.tcl integration tests + strEncoding fix#34
Merged
Merged
Conversation
ikolomi
commented
Jun 17, 2026
ikolomi
commented
Jun 17, 2026
ikolomi
commented
Jun 17, 2026
ikolomi
commented
Jun 17, 2026
ikolomi
commented
Jun 17, 2026
ikolomi
commented
Jun 17, 2026
ikolomi
commented
Jun 17, 2026
ikolomi
commented
Jun 17, 2026
9c14ef8 to
8d81d18
Compare
02e8ed7 to
0c7f36b
Compare
… infra (R2.3.10) (#33) Implements the minimal preshared-dictionary import surface so integration tests can run before S1.x training lands. R2.3.10 + §4.5 in the design doc: operator base64-encodes a ZSTD-trained dictionary and installs it via `COMPRESSION DICT-IMPORT <base64-bytes>`. The new dict is promoted as active; the previous active is retired through the existing registry path. The blocker this solves: `compressionEnqueueCandidate` early-returns when `compressionRegistryActive() == NULL`, so without a trained dict the entire write/sweep path is a no-op. Tests that exercise end-to-end compression behaviour (S2.7 write hook, S2.8 read hook, S2.9 sweeper) need a way to install a dict; this PR is that way. S1.x's full training implementation (BIO_COMPRESSION_TRAIN + ZDICT_trainFromBuffer on the bio thread) lands separately on the @GilboaAWS track. Implementation -------------- Hyphenated subcommand `DICT-IMPORT` (CLUSTER COUNT-FAILURE-REPORTS precedent — RESP doesn't have nested subcommand containers). Validation, in order: - Base64 decoding (private static `base64Decode` in compression.c; standard alphabet, optional `=` padding, whitespace rejected). - 4-byte ZSTD magic 0xEC30A437 — rejects raw-content prefixes and other non-trained bytes. Exotic operators with raw prefixes will have to find another route; the 99% case is "I trained a dict, I'm importing it" and that case wants real validation. - `ZSTD_createCDict` / `ZSTD_createDDict` — these accept arbitrary bytes as raw prefixes (never return NULL on garbage), so the magic check above is the actual content-validity gate. The ZSTD calls remain as belt-and-suspenders for OOM and similar. - `compressionRegistryAdd(pair, promote=1)` — same path a trained dict will use once S1.x lands. Reply: integer dict_id on success, RESP error on rejection. INFO renderer ------------- Replaced the `compression_active_dict_id:0` and `compression_known_ dicts:0` placeholders with live values via the new registry accessor `compressionRegistryGetKnownCount()`. Operators running this server can now see imported dicts immediately in `INFO compression` / `COMPRESSION STATUS`. Other field placeholders (compressed_objects, ratio, etc.) remain at 0 until later S2 PRs land their counters. Test infrastructure: runtime dict generation -------------------------------------------- Static dict fixtures don't scale to the test matrix the project needs (per-shape dicts for JSON / kv / log workloads, drift testing where the dict was trained on shape A but workload arrives as shape B, retraining cycles where dict A is replaced by dict B). Shipping multiple ~10 KiB binaries under tests/assets/ would bloat the repo and still not cover the drift case. Instead, we generate dicts at test time, on demand, parametrized by data shape. The dict generator MUST be external to valkey-server. If we used a server-side test command (e.g. DEBUG COMPRESSION TRAIN-FROM-BYTES), a bug in the server's training plumbing could mask itself — both the test fixture and the production training path would share code and exhibit the same bug. The infrastructure landed here uses a separate process that calls only ZDICT_trainFromBuffer directly: - tests/helpers/gen-zstd-dict.c (new): Standalone helper binary. Reads samples from stdin in a simple binary protocol (4-byte big-endian length + N bytes per sample, repeated until EOF), trains a ZSTD dictionary via ZDICT_trainFromBuffer, writes the trained dict to a path passed on argv. Links against the same vendored deps/zstd/libzstd.a as valkey-server, so ZDICT API behaviour matches what production will use, but runs in a separate process with no shared memory or globals with the SUT. - src/Makefile: Adds tests/helpers/gen-zstd-dict to ALL_BUILD_PREREQUISITES when BUILD_ZSTD=yes (gated by the same ifeq block that controls the feature itself). BUILD_ZSTD=no skips it — there's no compression feature to test. clean target updated. - tests/support/compression-helpers.tcl (new): Sample generators (gen_kv_samples, gen_json_samples, gen_log_samples) producing reproducible per-seed sample lists. gen_drifted_samples mixes two shapes by a `drift` fraction in [0,1] for drift / retraining tests. train_dict_from_samples pipes samples through the helper binary; import_dict is the convenience wrapper that trains + base64-encodes + sends COMPRESSION DICT-IMPORT. - tests/test_helper.tcl: source the new support file. - tests/unit/type/compression.tcl: the existing "import a real trained dict" Tcl test now generates samples + trains at test time instead of reading a static fixture. New "drift mixer sanity" test verifies the gen_drifted_samples helper itself. - tests/assets/test-compression.dict: deleted (no longer needed). Tests ----- gtest (392 total, +1 new under CompressionRegistryTest): - GetKnownCountTracksAddsAndCapEnforcement — verifies the new accessor moves with each Add and stays at the cap on rejection. Tcl (28 total, +6 new under unit/type/compression): - Rejects malformed base64. - Rejects valid base64 without ZSTD magic ("hello world"). - Rejects payloads smaller than the magic header. - Validates arity at the command-table level. - Imports a real runtime-trained dict, verifies INFO reflects active_dict_id+known_dicts, second import promotes new + retires previous (count=2). - Smoke-tests gen_drifted_samples (verifies pure-A / pure-B / 50-50 mixing produce shape-distinct outputs). Verification ------------ - 392 gtests pass (was 391 — +1 new). - 28 Tcl tests pass (was 22 — +6 new). - BUILD_ZSTD=yes and BUILD_ZSTD=no both clean with -Werror. - gen-zstd-dict helper builds only when BUILD_ZSTD=yes and is invoked correctly by the Tcl wrapper end-to-end. Out of scope for this PR ------------------------ - DICT-EXPORT (R2.3.10 mentions both; symmetric implementation is a small follow-up once we have one operator who needs it). - DICT-LIST / DICT-DROP (§4.5; pending S4.x observability work). - Real training (S1.x — @GilboaAWS track). - Topic-2 PR-B (compression-stress.tcl) — the integration stress test that USES this command. Lands next.
3ece0f0 to
a12430f
Compare
ikolomi
commented
Jun 18, 2026
| dict set expected_vals $i $v | ||
| lappend keys "fs$i" | ||
| } | ||
| assert_equal "raw" [r object encoding fs0] |
Owner
Author
There was a problem hiding this comment.
Same fix as test 2 — replaced assert_equal "raw" [r object encoding fs0] with a loop over all 50 keys before the COMPRESSION SWEEP FORCE. All-keys assertion both pre-sweep (RAW) and post-sweep (compressed + round-trip) now.
ikolomi
commented
Jun 18, 2026
ikolomi
commented
Jun 18, 2026
…ite fixes Adds tests/integration/compression.tcl — first end-to-end exercise of the merged S2.x stack against a real workload. Builds on top of PR-A (#33)'s runtime dict-generation infrastructure: each test imports a freshly-trained dict via tests/support/compression-helpers.tcl, then exercises one specific behaviour of the hot path. Six test cases: 1. Write-path round trip. master=compression + sweeper=disabled. SET a compressible value, poll until OBJECT ENCODING reports "compressed", verify GET round-trips the original bytes through the read-path transient view (R2.5.7). 2. Sweeper compresses pre-existing keys. master=off + populate 100 RAW keys, then flip to master=compression + sweeper=enabled. Wait for ALL keys compressed; verify EVERY value round-trips (no spot-checks). 3. Decompression drain. Continuing from #2's compressed state, flip to master=decompression + sweeper=enabled. Wait for ALL keys drained back to RAW; verify EVERY value round-trips. 4. COMPRESSION SWEEP FORCE end-to-end. master=compression + sweeper=disabled (manual-only). Populate uncompressed, then run COMPRESSION SWEEP FORCE. Wait for ALL keys to be compressed by a single forced pass; verify EVERY value round-trips. 5. Mixed workload preserves data integrity under live sweeper. master=compression + sweeper=enabled + 50% pacing. Populate 200 keys, run 500 random ops (GET/SET/APPEND/SETRANGE/DEL). 6. Ineligibility — values outside the size envelope and hot keys. Verifies the eligibility predicate (R2.2): values below compression-min-value-size, values above compression-max-value-size, and freshly-written hot keys must NOT be compressed even with the sweeper running at maximum cadence. Prerequisite fix 1: src/object.c strEncoding() ================================================ OBJECT ENCODING was returning "unknown" for compressed values because strEncoding() didn't have a case for OBJ_ENCODING_COMPRESSED. Design R2.7.1 requires it returns "compressed". One-line fix that slipped through earlier S2 PRs. Prerequisite fix 2: src/compression.c compressionEnqueueCandidate() ==================================================================== Use-after-free caught by AddressSanitizer on the first PR-B CI run. The eligibility predicate accepts encoding==RAW; a value currently in transient-view state (R2.5.7) reads as RAW because val_ptr is the per-iteration temp uncompressed sds. compressionEnqueueCandidate would then capture job->src = temp_sds, which restoreTransientEntry frees at beforeSleep — leaving the worker thread's job->src dangling into freed memory. Fix: gate the enqueue on !transientViewActive(value). Skipping the enqueue is functionally harmless — a value in transient view is already compressed (the original frame is saved in the side-map and will be restored at beforeSleep). One line added at the top of compressionEnqueueCandidate, with an explanatory comment naming the exact ASan trace it fixes. Tests: 392 gtests + 34 Tcl tests pass (28 unit + 6 integration). Both BUILD_ZSTD={yes,no} build clean with -Werror. Verified the asan fix locally by rebuilding with -fsanitize=address and re-running the integration suite — no use-after-free.
a12430f to
41db8ab
Compare
ikolomi
added a commit
that referenced
this pull request
Jun 18, 2026
* [Topic-2 PR-A] COMPRESSION DICT-IMPORT + runtime dict-generation test infra (R2.3.10) Implements the minimal preshared-dictionary import surface so integration tests can run before S1.x training lands. R2.3.10 + §4.5 in the design doc: operator base64-encodes a ZSTD-trained dictionary and installs it via `COMPRESSION DICT-IMPORT <base64-bytes>`. The new dict is promoted as active; the previous active is retired through the existing registry path. The blocker this solves: `compressionEnqueueCandidate` early-returns when `compressionRegistryActive() == NULL`, so without a trained dict the entire write/sweep path is a no-op. Tests that exercise end-to-end compression behaviour (S2.7 write hook, S2.8 read hook, S2.9 sweeper) need a way to install a dict; this PR is that way. S1.x's full training implementation (BIO_COMPRESSION_TRAIN + ZDICT_trainFromBuffer on the bio thread) lands separately on the @GilboaAWS track. Implementation -------------- Hyphenated subcommand `DICT-IMPORT` (CLUSTER COUNT-FAILURE-REPORTS precedent — RESP doesn't have nested subcommand containers). Validation, in order: - Base64 decoding (private static `base64Decode` in compression.c; standard alphabet, optional `=` padding, whitespace rejected). - 4-byte ZSTD magic 0xEC30A437 — rejects raw-content prefixes and other non-trained bytes. Exotic operators with raw prefixes will have to find another route; the 99% case is "I trained a dict, I'm importing it" and that case wants real validation. - `ZSTD_createCDict` / `ZSTD_createDDict` — these accept arbitrary bytes as raw prefixes (never return NULL on garbage), so the magic check above is the actual content-validity gate. The ZSTD calls remain as belt-and-suspenders for OOM and similar. - `compressionRegistryAdd(pair, promote=1)` — same path a trained dict will use once S1.x lands. Reply: integer dict_id on success, RESP error on rejection. INFO renderer ------------- Replaced the `compression_active_dict_id:0` and `compression_known_ dicts:0` placeholders with live values via the new registry accessor `compressionRegistryGetKnownCount()`. Operators running this server can now see imported dicts immediately in `INFO compression` / `COMPRESSION STATUS`. Other field placeholders (compressed_objects, ratio, etc.) remain at 0 until later S2 PRs land their counters. Test infrastructure: runtime dict generation -------------------------------------------- Static dict fixtures don't scale to the test matrix the project needs (per-shape dicts for JSON / kv / log workloads, drift testing where the dict was trained on shape A but workload arrives as shape B, retraining cycles where dict A is replaced by dict B). Shipping multiple ~10 KiB binaries under tests/assets/ would bloat the repo and still not cover the drift case. Instead, we generate dicts at test time, on demand, parametrized by data shape. The dict generator MUST be external to valkey-server. If we used a server-side test command (e.g. DEBUG COMPRESSION TRAIN-FROM-BYTES), a bug in the server's training plumbing could mask itself — both the test fixture and the production training path would share code and exhibit the same bug. The infrastructure landed here uses a separate process that calls only ZDICT_trainFromBuffer directly: - tests/helpers/gen-zstd-dict.c (new): Standalone helper binary. Reads samples from stdin in a simple binary protocol (4-byte big-endian length + N bytes per sample, repeated until EOF), trains a ZSTD dictionary via ZDICT_trainFromBuffer, writes the trained dict to a path passed on argv. Links against the same vendored deps/zstd/libzstd.a as valkey-server, so ZDICT API behaviour matches what production will use, but runs in a separate process with no shared memory or globals with the SUT. - src/Makefile: Adds tests/helpers/gen-zstd-dict to ALL_BUILD_PREREQUISITES when BUILD_ZSTD=yes (gated by the same ifeq block that controls the feature itself). BUILD_ZSTD=no skips it — there's no compression feature to test. clean target updated. - tests/support/compression-helpers.tcl (new): Sample generators (gen_kv_samples, gen_json_samples, gen_log_samples) producing reproducible per-seed sample lists. gen_drifted_samples mixes two shapes by a `drift` fraction in [0,1] for drift / retraining tests. train_dict_from_samples pipes samples through the helper binary; import_dict is the convenience wrapper that trains + base64-encodes + sends COMPRESSION DICT-IMPORT. - tests/test_helper.tcl: source the new support file. - tests/unit/type/compression.tcl: the existing "import a real trained dict" Tcl test now generates samples + trains at test time instead of reading a static fixture. New "drift mixer sanity" test verifies the gen_drifted_samples helper itself. - tests/assets/test-compression.dict: deleted (no longer needed). Tests ----- gtest (392 total, +1 new under CompressionRegistryTest): - GetKnownCountTracksAddsAndCapEnforcement — verifies the new accessor moves with each Add and stays at the cap on rejection. Tcl (28 total, +6 new under unit/type/compression): - Rejects malformed base64. - Rejects valid base64 without ZSTD magic ("hello world"). - Rejects payloads smaller than the magic header. - Validates arity at the command-table level. - Imports a real runtime-trained dict, verifies INFO reflects active_dict_id+known_dicts, second import promotes new + retires previous (count=2). - Smoke-tests gen_drifted_samples (verifies pure-A / pure-B / 50-50 mixing produce shape-distinct outputs). Verification ------------ - 392 gtests pass (was 391 — +1 new). - 28 Tcl tests pass (was 22 — +6 new). - BUILD_ZSTD=yes and BUILD_ZSTD=no both clean with -Werror. - gen-zstd-dict helper builds only when BUILD_ZSTD=yes and is invoked correctly by the Tcl wrapper end-to-end. Out of scope for this PR ------------------------ - DICT-EXPORT (R2.3.10 mentions both; symmetric implementation is a small follow-up once we have one operator who needs it). - DICT-LIST / DICT-DROP (§4.5; pending S4.x observability work). - Real training (S1.x — @GilboaAWS track). - Topic-2 PR-B (compression-stress.tcl) — the integration stress test that USES this command. Lands next. * [Topic-2 PR-B] compression-stress.tcl integration tests + strEncoding fix (#34) * [Topic-2 PR-A] COMPRESSION DICT-IMPORT + runtime dict-generation test infra (R2.3.10) (#33) Implements the minimal preshared-dictionary import surface so integration tests can run before S1.x training lands. R2.3.10 + §4.5 in the design doc: operator base64-encodes a ZSTD-trained dictionary and installs it via `COMPRESSION DICT-IMPORT <base64-bytes>`. The new dict is promoted as active; the previous active is retired through the existing registry path. The blocker this solves: `compressionEnqueueCandidate` early-returns when `compressionRegistryActive() == NULL`, so without a trained dict the entire write/sweep path is a no-op. Tests that exercise end-to-end compression behaviour (S2.7 write hook, S2.8 read hook, S2.9 sweeper) need a way to install a dict; this PR is that way. S1.x's full training implementation (BIO_COMPRESSION_TRAIN + ZDICT_trainFromBuffer on the bio thread) lands separately on the @GilboaAWS track. Implementation -------------- Hyphenated subcommand `DICT-IMPORT` (CLUSTER COUNT-FAILURE-REPORTS precedent — RESP doesn't have nested subcommand containers). Validation, in order: - Base64 decoding (private static `base64Decode` in compression.c; standard alphabet, optional `=` padding, whitespace rejected). - 4-byte ZSTD magic 0xEC30A437 — rejects raw-content prefixes and other non-trained bytes. Exotic operators with raw prefixes will have to find another route; the 99% case is "I trained a dict, I'm importing it" and that case wants real validation. - `ZSTD_createCDict` / `ZSTD_createDDict` — these accept arbitrary bytes as raw prefixes (never return NULL on garbage), so the magic check above is the actual content-validity gate. The ZSTD calls remain as belt-and-suspenders for OOM and similar. - `compressionRegistryAdd(pair, promote=1)` — same path a trained dict will use once S1.x lands. Reply: integer dict_id on success, RESP error on rejection. INFO renderer ------------- Replaced the `compression_active_dict_id:0` and `compression_known_ dicts:0` placeholders with live values via the new registry accessor `compressionRegistryGetKnownCount()`. Operators running this server can now see imported dicts immediately in `INFO compression` / `COMPRESSION STATUS`. Other field placeholders (compressed_objects, ratio, etc.) remain at 0 until later S2 PRs land their counters. Test infrastructure: runtime dict generation -------------------------------------------- Static dict fixtures don't scale to the test matrix the project needs (per-shape dicts for JSON / kv / log workloads, drift testing where the dict was trained on shape A but workload arrives as shape B, retraining cycles where dict A is replaced by dict B). Shipping multiple ~10 KiB binaries under tests/assets/ would bloat the repo and still not cover the drift case. Instead, we generate dicts at test time, on demand, parametrized by data shape. The dict generator MUST be external to valkey-server. If we used a server-side test command (e.g. DEBUG COMPRESSION TRAIN-FROM-BYTES), a bug in the server's training plumbing could mask itself — both the test fixture and the production training path would share code and exhibit the same bug. The infrastructure landed here uses a separate process that calls only ZDICT_trainFromBuffer directly: - tests/helpers/gen-zstd-dict.c (new): Standalone helper binary. Reads samples from stdin in a simple binary protocol (4-byte big-endian length + N bytes per sample, repeated until EOF), trains a ZSTD dictionary via ZDICT_trainFromBuffer, writes the trained dict to a path passed on argv. Links against the same vendored deps/zstd/libzstd.a as valkey-server, so ZDICT API behaviour matches what production will use, but runs in a separate process with no shared memory or globals with the SUT. - src/Makefile: Adds tests/helpers/gen-zstd-dict to ALL_BUILD_PREREQUISITES when BUILD_ZSTD=yes (gated by the same ifeq block that controls the feature itself). BUILD_ZSTD=no skips it — there's no compression feature to test. clean target updated. - tests/support/compression-helpers.tcl (new): Sample generators (gen_kv_samples, gen_json_samples, gen_log_samples) producing reproducible per-seed sample lists. gen_drifted_samples mixes two shapes by a `drift` fraction in [0,1] for drift / retraining tests. train_dict_from_samples pipes samples through the helper binary; import_dict is the convenience wrapper that trains + base64-encodes + sends COMPRESSION DICT-IMPORT. - tests/test_helper.tcl: source the new support file. - tests/unit/type/compression.tcl: the existing "import a real trained dict" Tcl test now generates samples + trains at test time instead of reading a static fixture. New "drift mixer sanity" test verifies the gen_drifted_samples helper itself. - tests/assets/test-compression.dict: deleted (no longer needed). Tests ----- gtest (392 total, +1 new under CompressionRegistryTest): - GetKnownCountTracksAddsAndCapEnforcement — verifies the new accessor moves with each Add and stays at the cap on rejection. Tcl (28 total, +6 new under unit/type/compression): - Rejects malformed base64. - Rejects valid base64 without ZSTD magic ("hello world"). - Rejects payloads smaller than the magic header. - Validates arity at the command-table level. - Imports a real runtime-trained dict, verifies INFO reflects active_dict_id+known_dicts, second import promotes new + retires previous (count=2). - Smoke-tests gen_drifted_samples (verifies pure-A / pure-B / 50-50 mixing produce shape-distinct outputs). Verification ------------ - 392 gtests pass (was 391 — +1 new). - 28 Tcl tests pass (was 22 — +6 new). - BUILD_ZSTD=yes and BUILD_ZSTD=no both clean with -Werror. - gen-zstd-dict helper builds only when BUILD_ZSTD=yes and is invoked correctly by the Tcl wrapper end-to-end. Out of scope for this PR ------------------------ - DICT-EXPORT (R2.3.10 mentions both; symmetric implementation is a small follow-up once we have one operator who needs it). - DICT-LIST / DICT-DROP (§4.5; pending S4.x observability work). - Real training (S1.x — @GilboaAWS track). - Topic-2 PR-B (compression-stress.tcl) — the integration stress test that USES this command. Lands next. * [Topic-2 PR-B] compression-stress.tcl integration tests + 2 prerequisite fixes Adds tests/integration/compression.tcl — first end-to-end exercise of the merged S2.x stack against a real workload. Builds on top of PR-A (#33)'s runtime dict-generation infrastructure: each test imports a freshly-trained dict via tests/support/compression-helpers.tcl, then exercises one specific behaviour of the hot path. Six test cases: 1. Write-path round trip. master=compression + sweeper=disabled. SET a compressible value, poll until OBJECT ENCODING reports "compressed", verify GET round-trips the original bytes through the read-path transient view (R2.5.7). 2. Sweeper compresses pre-existing keys. master=off + populate 100 RAW keys, then flip to master=compression + sweeper=enabled. Wait for ALL keys compressed; verify EVERY value round-trips (no spot-checks). 3. Decompression drain. Continuing from #2's compressed state, flip to master=decompression + sweeper=enabled. Wait for ALL keys drained back to RAW; verify EVERY value round-trips. 4. COMPRESSION SWEEP FORCE end-to-end. master=compression + sweeper=disabled (manual-only). Populate uncompressed, then run COMPRESSION SWEEP FORCE. Wait for ALL keys to be compressed by a single forced pass; verify EVERY value round-trips. 5. Mixed workload preserves data integrity under live sweeper. master=compression + sweeper=enabled + 50% pacing. Populate 200 keys, run 500 random ops (GET/SET/APPEND/SETRANGE/DEL). 6. Ineligibility — values outside the size envelope and hot keys. Verifies the eligibility predicate (R2.2): values below compression-min-value-size, values above compression-max-value-size, and freshly-written hot keys must NOT be compressed even with the sweeper running at maximum cadence. Prerequisite fix 1: src/object.c strEncoding() ================================================ OBJECT ENCODING was returning "unknown" for compressed values because strEncoding() didn't have a case for OBJ_ENCODING_COMPRESSED. Design R2.7.1 requires it returns "compressed". One-line fix that slipped through earlier S2 PRs. Prerequisite fix 2: src/compression.c compressionEnqueueCandidate() ==================================================================== Use-after-free caught by AddressSanitizer on the first PR-B CI run. The eligibility predicate accepts encoding==RAW; a value currently in transient-view state (R2.5.7) reads as RAW because val_ptr is the per-iteration temp uncompressed sds. compressionEnqueueCandidate would then capture job->src = temp_sds, which restoreTransientEntry frees at beforeSleep — leaving the worker thread's job->src dangling into freed memory. Fix: gate the enqueue on !transientViewActive(value). Skipping the enqueue is functionally harmless — a value in transient view is already compressed (the original frame is saved in the side-map and will be restored at beforeSleep). One line added at the top of compressionEnqueueCandidate, with an explanatory comment naming the exact ASan trace it fixes. Tests: 392 gtests + 34 Tcl tests pass (28 unit + 6 integration). Both BUILD_ZSTD={yes,no} build clean with -Werror. Verified the asan fix locally by rebuilding with -fsanitize=address and re-running the integration suite — no use-after-free.
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
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.
Summary
Adds
tests/integration/compression.tcl— first end-to-end exercise of the merged S2.x stack against a real workload. Builds on top of PR-A (#33)'s runtime dict-generation infrastructure: each test imports a freshly-trained dict viatests/support/compression-helpers.tcl, then exercises one specific behaviour of the hot path.PR-A is now merged to
unstable; this PR is rebased on top.Test cases (6 total)
master=compression + sweeper=disabled→SETenqueues → worker drain installs compressed frame →OBJECT ENCODINGreportscompressed→GETreturns original bytes via the transient-view read path (R2.5.7)master=off, flip tomaster=compression + sweeper=enabled, single-pass sweep compresses ALL keys; EVERY value round-trips (no spot-checks)master=decompression + sweeper=enabled(auto-retires the active dict per R2.1.5), sweeper callscompressionPermanentlyDecompressper key; ALL keys end uprawAND every value round-tripsCOMPRESSION SWEEP FORCEend-to-endmaster=compression + sweeper=disabled(manual-only), populate uncompressed,COMPRESSION SWEEP FORCE, ALL keys compressed by a single forced pass; EVERY value round-tripscompression-min-value-size, values abovecompression-max-value-size, and freshly-written hot keys (idle_secs <compression-min-idle-seconds) must NOT be compressed even with the sweeper running at maximum cadence; control case proves the sweeper isn't merely crashedAll 6 skip cleanly under
BUILD_ZSTD=no(helper binary not built).Two prerequisite fixes folded in
This PR depends on two correctness fixes that surfaced when the integration tests first ran. Each is a small, surgical change documented inline.
1.
src/object.c—strEncoding()missing case forOBJ_ENCODING_COMPRESSEDOBJECT ENCODING <key>was returning"unknown"for compressed values becausestrEncoding()had no case forOBJ_ENCODING_COMPRESSED. Design R2.7.1 explicitly requires it returns"compressed". One-line fix; should have landed withcreateCompressedObjectin S2.5; slipped through. Every test in this file pollsOBJECT ENCODING, so this is a hard prerequisite.2.
src/compression.c—compressionEnqueueCandidateuse-after-freeASan caught this on the first PR-B sanitizer run.
Trigger: the sweeper. Sequence within a single event-loop iteration:
The write/modify path is not the trigger — writes go through
lookupKeyWrite→compressionPermanentlyDecompress, which kills any transient view before mutation; subsequentsignalModifiedKeyenqueues the post-COW robj (fresh sds, nevertemp_sds). The sweeper iterates the kvstore directly with no transient-view awareness, so it grabs values whoseval_ptris currently the per-iteration temp sds.Fix: gate
compressionEnqueueCandidateon!transientViewActive(value). Skipping is functionally identical because a value in transient view is already compressed (the original frame is alive in the side-map and gets restored at the nextbeforeSleep). Single chokepoint protects all current and future enqueue paths.Polling strategy
compression_compressed_objectsinINFOis currently hardcoded to:0(the renderer comment says "stays 0 until later S2 PRs land their counters" — depends on encode-path counter wiring tracked separately in the S4.x track). Tests therefore pollOBJECT ENCODINGper-key instead, via three helpers:The helpers use
wait_for_conditionwith a 50ms × 200-tries default budget, giving the server's event loop time to drain the worker outbox between polls. Each helper carries an explicitTODO(S4.x)annotation per the project's "TODO-mark suboptimal/superseded code" rule. SameTODO(S4.x)onassert_no_compression_errorssincecompression_errors_totalis also currently hardcoded to 0.Verification
unit/type/compression(PR-A) + 6 newintegration/compression.BUILD_ZSTD=yesandBUILD_ZSTD=noboth compile clean with-Werror.-fsanitize=addressand re-running the integration suite — no UAF.BUILD_ZSTD=noTcl path skips the helper-dependent tests by design).Out of scope for this PR
compression_compressed_objects(and friends) to real registry state — separate S4.x ticket.