opt: allow bitCost == scaleLog in ZSTD_rescaleFreqs (fixes #4681) - #4749
Open
tvost2 wants to merge 1 commit into
Open
opt: allow bitCost == scaleLog in ZSTD_rescaleFreqs (fixes #4681)#4749tvost2 wants to merge 1 commit into
tvost2 wants to merge 1 commit into
Conversation
) When the optimal parser initializes its statistics from a dictionary's FSE tables, it reads a per-symbol cost with FSE_getMaxNbBits(). That function returns a "fake cost" of tableLog+1 for zero-frequency symbols. The litLength / matchLength FSE tables can use tableLog == 9 == scaleLog-1, so the fake cost is exactly scaleLog (10), which tripped assert(bitCost < scaleLog) in debug builds. The value computation right below already copes with it: 1 << (scaleLog - bitCost) == 1 << 0 == 1, i.e. identical to the release-build result and to the 'minimum to calculate cost' fallback. Relax the three FSE asserts to bitCost <= scaleLog, matching the literal loop just above which already uses HUF_getNbBitsFromCTable() the same way. Adds a fuzzer.c regression test that builds a CDict with an optimal-parser strategy from a 433-byte dictionary whose FSE NCount has zero-frequency matchlength symbols, then compresses and round-trips.
tvost2
force-pushed
the
fix/opt-rescalefreqs-assert
branch
from
August 31, 2026 03:16
4635578 to
4717a4f
Compare
tvost2
marked this pull request as ready for review
August 31, 2026 03:17
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.
Fixes #4681
ZSTD_rescaleFreqs()initializes the optimal parser's symbol statistics from adictionary's entropy tables. For the litLength / matchLength / offCode streams it
reads a per-symbol cost with
FSE_getMaxNbBits(), which by contract returns a"fake cost" of
tableLog + 1for zero-frequency symbols (lib/common/fse.h).LLFSELog/MLFSELogare9andscaleLoghere is10, so a dictionarywhose FSE
NCountleaves a symbol in0..MaxLL/0..MaxMLat frequency 0makes
bitCost == scaleLog, trippingin
DEBUGLEVEL>=1builds. Release builds are unaffected: the next line computes1 << (scaleLog - bitCost)=1 << 0=1, which is exactly the/* minimum to calculate cost */fallback, so the statistics are already correctfor this case.
The literal loop immediately above already handles the analogous Huffman
case with
assert(bitCost <= scaleLog). This makes the three FSE assertsconsistent with it (
<→<=); no behavior change beyond the assert.Reproducer (from #4681, Aisle Research)
446-byte input to a
dictionary_loader-style harness drivingZSTD_createCDict_advancedwith an optimal-parser strategy:Confirmed locally (
DEBUGLEVEL=1): aborts before the change,[*] no crashafter.
Test
fuzzer.cgainstest : optimal parser + dictionary with sparse FSE tables:build a CDict with each optimal-parser strategy (
btopt/btultra/btultra2)from the 433-byte dictionary embedded in that reproducer (its FSE matchlength
NCounthas zero-frequency symbols), then compress + round-trip. Fails at theassert on
dev, passes with this change.basicUnitTestsgreen end-to-end.Note
#4681 also suggests option (b): gate the FSE branch on
fse.matchlength_repeatMode == FSE_repeat_valid(etc.) and fall through to theno-dict-stats path per stream when the dict
NCountwas not fully valid. That ismore principled but more invasive; happy to follow up with it if preferred. This
PR is the minimal correctness fix.
AI-assisted; reproduced, fixed, built and tested locally by me, and I take
responsibility for the change.