Repository navigation
feat(internal): make level govern match-finding search depth - #52
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus 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 |
a5df1c5 to
e13a167
Compare
e13a167 to
bf40dae
Compare
bf40dae to
5401ac2
Compare
5401ac2 to
8f4d101
Compare
9f92db6 to
24688f7
Compare
17521d4 to
b46dff5
Compare
b46dff5 to
cb36d6c
Compare
`level` (1..22, zstd's public range) was a complete no-op: the encoder always used the same fixed maxChain/maxCandidates regardless of what was passed. It now maps to a real search-depth knob via a new searchDepthFor(level), governing ONLY how many candidate distances the matcher examines per position -- not entropy-table choice (dict-entropy reuse from the Repeat-mode/Treeless-literals work stays unconditional whenever the dict's tables are covered, independent of level) and not zstd's other 21 levels' distinct strategies/parameters (windowLog, targetLength, etc.) -- this encoder still uses one fixed greedy/lazy strategy at every level, just searching more or less. level == 19 (Zstd.DEFAULT_LEVEL) maps to exactly (maxChain=64, maxCandidates=32) -- today's previously-hardcoded values -- so compressing at the default level is byte-identical to before this change; ByteIdenticalRegressionTest needed zero hex changes, which is the actual proof this held. Below 19 it interpolates down to a fast floor (8, 4) at level 1; at/above 19 it interpolates up to a capped ceiling (256, 128) at level 22, deliberately capped rather than left to keep growing, to bound worst-case CPU on a highly repetitive payload at high effort. Out-of-range level values are clamped rather than rejected, matching real zstd's tolerance for requests outside its advertised range. MatchIndex.forEachCandidate gained a maxCandidates parameter (defaulted, so any other caller is unaffected) instead of always reading the compile-time MAX_CANDIDATES constant -- the search depth is a per-call knob, not part of the index structure, so the same index (built once per dictionary) keeps getting reused across calls at different levels without rebuilding it. Tests: direct checks of searchDepthFor's mapping at the floor (1), default (19), and ceiling (22), out-of-range clamping, and monotonicity across the whole 1..22 range. Round-trip checks at several levels confirm every level still produces a valid, decodable frame (deeper search can only change which match gets chosen, never produce an invalid sequence). An adoption guard compresses a purpose-built input -- a target run sandwiched around 60 pairwise- distinct decoy chunks sharing only its 4-byte hash prefix, forcing a genuine 61-entry-deep hash-chain walk -- and confirms level 22 finds the real match level 1's shallower floor cannot reach, producing a strictly smaller frame; this is the actual proof `level` does something, not just that it's plumbed through without crashing. Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com>
cb36d6c to
6ad7381
Compare
1 participant
Summary
level(1..22, zstd's public range) was a complete no-op: the encoderalways used the same fixed match-finding effort regardless of what was
passed. It now maps to a real search-depth knob, governing only how
many candidate distances the matcher examines per position.
Type of change
Related issue / discussion
Last PR in the encoder-parity series (#47/#48/#50/#51). Independent of
the others — this is a search-depth change only, unrelated to
dict-entropy reuse.
Affirmations
git commit -s).CONTRIBUTING.md../gradlew apiDumpand committed the regeneratedapi/*.apifiles. — N/A,level's signature was already public; only its (documented no-op) behavior changes../gradlew jvmTest)../gradlew buildlocally and it passes.How was this verified?
level == 19(Zstd.DEFAULT_LEVEL) maps to exactly the previouslyhardcoded values, so compressing at the default level is
byte-identical to before this change —
ByteIdenticalRegressionTestneeded zero hex changes, which is the actual proof this held.
Below 19 it interpolates down to a fast floor at level 1; at/above 19
it interpolates up to a deliberately capped ceiling at level 22
(bounding worst-case CPU on a highly repetitive payload at high
effort, rather than left to keep growing). Out-of-range values are
clamped rather than rejected, matching real zstd's tolerance.
and monotonicity across the whole 1..22 range.
every level still produces a valid, decodable frame — deeper search
can only change which match gets chosen, never produce an invalid
sequence.
sandwiched around 60 pairwise-distinct decoy chunks sharing only its
4-byte hash prefix, forcing a genuine 61-entry-deep hash-chain walk)
and confirms level 22 finds the real match level 1's shallower floor
cannot reach, producing a strictly smaller frame — the actual proof
leveldoes something, not just that it's plumbed through withoutcrashing.
./gradlew jvmTestand./gradlew build(full 13-target matrix)both pass.
Notes for reviewers
MatchIndex.forEachCandidategained a defaultedmaxCandidatesparameter instead of always reading the compile-time constant — the
search depth is a per-call knob, not part of the index structure, so
the same index (built once per dictionary) keeps getting reused across
calls at different levels without rebuilding it.