perf(deflate): relax each match length at its own nearest distance - #504
Open
justin13888 wants to merge 3 commits into
Open
perf(deflate): relax each match length at its own nearest distance#504justin13888 wants to merge 3 commits into
justin13888 wants to merge 3 commits into
Conversation
`parse_dp` relaxed every length from `MIN_MATCH` to the longest match at the longest match's distance, so a short match had to pay a far distance code the cost model had every reason to price high. zopfli's `ZopfliFindLongestMatch` instead records, per length, the nearest distance that reaches it. `Matcher::find_sublen` runs the existing nearest-first chain walk and fills a caller-owned `[u16; MAX_MATCH + 1]` in which `sublen[len]` is the nearest distance achieving at least `len`: a candidate writes exactly `best_len + 1..=len`, the lengths it is the first to reach. `find` keeps its signature and shares the walk with no table, so the lazy parser pays nothing and is byte-identical. `parse_dp` prices each length at `sublen[len]`, re-deriving the distance cost only when the distance changes along the length axis; the table lives once per span. `Level::Best` output (zlib streams, `cargo bench -p gamut-deflate --bench compression` size table, before -> after): rfc1951.txt 10767 -> 10664 (gap to zopfli +2.1% -> +1.1%), lz77.rs at the #478 head 8055 -> 8003 (+1.3% -> +0.7%); text.x300 103, ramp20k 399 and pseudo20k 2236 unchanged. `tests/default_bytes.rs` re-pinned the two rows that moved, both down: skewed Best 1061 -> 1048, mixed Best 21643 -> 21601; no row grew at any level and every Fast/Default pin is unchanged. Best is up to ~8% slower on the rfc1950.txt throughput input (22.88 -> 24.68 ms fastest), within a session whose matcher-free Store row also moved 7%; Default/Fast within that noise. Closes #479
The independent review of #479 found two documentation errors: STATUS.md said the ramp20k bench fixture's matches sit at distance 1, but the fixture is i % 256, so they sit at distance 256 (one period; the conclusion that a short length's nearest distance is the longest match's stands). README's lz77.rs row compressed whatever the file was at bench time and drifted again under rustfmt, so it is pinned to the file at the Refs #479
…hared walk Review of #479 decided two documentation points. README said the whole size table is reproduced by cargo bench while pinning its lz77.rs row to the file at 4f2c2a4, which no later bench reproduces; the claim now excludes that row and the note says what reproduces it. STATUS.md's Throughput section still called the Best-level chain loop Matcher::find; it is Matcher::walk, shared by find (lazy parser) and find_sublen (optimal parse). Refs #479
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.
Stacked on #499 (
perf/478-deflate-u64-compare); this PR's base is that branch so its diff shows only this change. Merge #499 first, then retarget or merge this one.This is an unattended run: no human approved the plan. The decision record below is what a human reads afterwards.
Summary
Matcher::find_sublenruns the existing nearest-first hash-chain walk and also fills a caller-owned[u16; MAX_MATCH + 1]in whichsublen[len]is the nearest distance achieving at leastlen(zopfli'ssublen): a candidate records exactly the lengths it is the first to reach, so the first to reach a length is the nearest that does.findkeeps its signature and delegates to the same walk with no table, so the lazy parser (Level::Fast/Default) pays nothing and is byte-identical.parse_dp(theLevel::Bestshortest-path pass) relaxes each length3..=longestatsublen[len]instead of at the longest match's distance, re-deriving the distance cost only when the distance changes along the length axis. The table is allocated once per span, not per position.Level::Best, zlib streams):rfc1951.txt10 767 → 10 664 (gap to zopfli +2.1% → +1.1%),lz77.rsat the gamut-deflate: compare 8 bytes at a time in longest_match #478 head 8 055 → 8 003 (+1.3% → +0.7%);text.x300/ramp20k/pseudo20kunchanged (their short lengths' nearest distance is the longest match's).tests/default_bytes.rs: twoBestpins moved, both down —skewed1061 → 1048,mixed21643 → 21601 — and no row grew at any level; everyFast/Defaultpin is unchanged.Level::Beston therfc1950.txtthroughput input 22.88 → 24.68 ms fastest (+7.9%; median +7.0%) in one back-to-back session where the matcher-freeStorerow also moved 7%, so that is an upper bound. Numbers and invocation recorded inSTATUS.md;README.mdtable updated.find_sublen_records_each_length_at_its_nearest_distance(nearer candidate reaches 3, farther reaches 5 →sublen[3]= near,sublen[4..=5]= far, rest 0, longest unchanged) andoptimal_parse_takes_a_short_match_at_its_nearer_distance(hand-set costs under which relaxing at the longest match's distance emitsMatch{5, 44}and per-length relaxation must emitMatch{3, 3}+ two literals).Validation
All commands ran in the lane worktree on commit
18c058f(this tree); crate-scoped runs withCARGO_BUILD_JOBS=2, workspace runs insidesystemd-run --user --scope --slice=agents.slice -p MemoryMax=16G -p MemorySwapMax=0 -- env CARGO_BUILD_JOBS=2 CMAKE_BUILD_PARALLEL_LEVEL=2 sh -c 'ulimit -v 12000000; …'.cargo test -p gamut-deflate --all-features— pass: 49 unit tests (incl. the two new ones),default_bytes1,oracle11 (incl.best_beats_zlib_9), 1 doctest. Before re-pinning, the same run reported exactly two moved pins (skewed/Best 1048,mixed/Best 21601), both smaller.__CARGO_TEST_ROOT=<worktree root> mise run fmtthen… mise run fmt-check— exit 0 (the prefix is the documented nested-worktree artefact; no manifest changed).mise run check-tests— pass ("module docs, pinned proptest seeds and oracle filenames all conform").convco check origin/perf/478-deflate-u64-compare..HEAD— "no errors in 1 commit".mise run lint(capped scope) — exit 0 (cargo clippy --workspace --all-targets --all-features -- -D warnings, 8m30s).mise run test(capped scope) — exit 0, no failing test binary.cargo bench -p gamut-deflate --bench compressionsize table, before (at4f2c2a4) and after; the like-for-likelz77.rsrow is the after binary run directly over the base revision of the file (my copy restored and SHA-256-verified afterwards). Throughput:cargo bench -p gamut-deflate --bench compression -- gamut, base code then this code, back to back. Numbers in the Summary and inSTATUS.md.GAMUT_MUTANTS_BASE=origin/perf/478-deflate-u64-compare mise run mutants-diff(capped scope) — "Found 34 mutants to test … 34 mutants tested in 49s: 27 caught, 7 unviable"; 0 missed, 0 timeouts, no exclusion added.cargo test -p gamut-deflate --all-featuresre-run — pass (same counts);__CARGO_TEST_ROOT=<worktree root> mise run fmt-check,mise run check-tests,convco check origin/perf/478-deflate-u64-compare..HEAD— pass.lint/test/mutants-diffwere not re-run: the second commit touches onlySTATUS.md/README.md, neither of which is compiled or included in crate docs (lib.rshas noinclude_str!).check-release-deps/check-ffi-features/check-ffi-header(noCargo.tomlor C surface touched);coverage(no new module; the new code is exercised by the existing suite and two inline tests).862c799(first two commits): Format & Metadata, Clippy & Doctests, Coverage (test gate), Incremental (PR diff) 0–3 — all passed.README.md/STATUS.mdwording):__CARGO_TEST_ROOT=<worktree root> mise run fmt-check,mise run check-tests,convco check origin/perf/478-deflate-u64-compare..HEAD— pass;lint/test/mutants-diffnot re-run (Markdown only).Risks and rollout
Level::Bestchange (smaller on every measured input;Fast/Default/Storebyte-identical). Any consumer pinning exactBestoutput must re-pin; inside the workspace onlygamut-deflate's owndefault_bytes.rsdoes, and it is re-pinned here.Level::Bestis up to ~8% slower on prose-like input (the per-position table clear and fill);Default/Fastunaffected. Reverting is one commit.unsafe.Issue
Closes #479
Decisions taken
Unresolved review notes
Independent review (a separate adversarial reading of the diff, reported after the first commit) raised four findings; dispositions:
ramp20k's matches are "all at distance 1"; the fixture isi % 256, so distance 256. Repaired (wording; the conclusion — one period, so the nearest distance for a short length is the longest match's — stands).lz77.rsrow was self-referential (the file changed again undermise run fmtafter the bench). Repaired: the row is pinned to the file at4f2c2a4, as STATUS.md already does, with a note.+→*mutant indcost = dist_cost[dsym] + dbitsis killed by thedefault_bytes.rspins, not by a named inline test. Not changed — see decision 10;mutants-diffconfirms it is caught.find_sublenclears the whole table per position to honour the "0 where none" contract. Not changed — decision 2 fixes that contract; see decision 11.Nothing is left for a human reviewer to decide from this pass.