Repository navigation
dictBuilder: break COVER sort ties by position, not by address - #4765
jaypatrickhoward wants to merge 2 commits into
Conversation
stableSort() is documented to leave each dmer group ordered by position in the input, and COVER_group() depends on it: it counts how many samples a dmer occurs in using a forward-only cursor, so a group whose positions are not ascending has occurrences silently dropped. COVER_strict_cmp() tried to provide that ordering by breaking ties on `lp < rp`. But lp and rp are the addresses of the elements being compared, not the positions they hold. Those coincide only until qsort() performs its first swap; afterwards the tie-break orders by where an element currently sits rather than by what it contains. The result is a comparator that is not a function of the values it compares, with three consequences: - Output is not reproducible. When qsort() compares an element against a temporary (a pivot copy, which is common), one operand is not in the array at all, so the comparison is stack-vs-heap and ASLR decides it. The same binary on the same input produced two different dictionaries in six consecutive runs. - Output depends on the C library. glibc happens to satisfy the invariant; Apple libc and MSVC do not, so they produce different dictionaries from the same input. - An inconsistent comparator breaks quicksort's partitioning assumptions. On MSVC, training on a 16 MB corpus at d=6 takes 465s; with this fix, 8s. Comparing the stored positions instead makes the key (dmer, position). Positions are unique, so no two elements compare equal, the order is total, and every conforming qsort() must produce the same arrangement. Verified: on glibc the output is unchanged (byte-identical across 215 configurations spanning 20 corpora), and on Apple libc and MSVC the output now matches what glibc produces.
| int result = COVER_cmp((COVER_ctx_t*)g_coverCtx, lp, rp); | ||
| if (result == 0) { | ||
| result = lp < rp ? -1 : 1; | ||
| result = *(const U32 *)lp < *(const U32 *)rp ? -1 : 1; |
There was a problem hiding this comment.
The existing implementation also makes a mistake with this, but please ensure when the values are equal, 0 is returned. Could you also add a test case for this to ensure the ordering is correct?
There was a problem hiding this comment.
Done in 8dbec80: both comparators now do a three-way compare on position and return 0 when the positions are equal.
I took "when the values are equal" to mean the two positions being equal (a self-comparison), not the two dmers. Returning 0 for equal dmers would remove the tie-break and bring back the ordering problem this PR fixes, so equal dmers still fall through to the position compare. Let me know if you meant something else.
Since suffix positions are unique, the 0 case is only reachable on a self-comparison and cannot reorder two distinct elements. I checked that the output is unchanged: byte-identical dictionaries before and after, across 72 corpus/k/d combinations.
For the test: the suffix array is not reachable from tests/, so it is a behavioral check in fuzzer.c. It trains COVER on low-entropy samples, where ties are everywhere, and compares the dictionary hash against the position-ordered result for both the d<=8 and d>8 comparator paths. Against the old address-based tie-break it fails on macOS with a different hash; with position ordering it passes. On glibc the stable mergesort happened to yield position order, so the old code passes there too, which is consistent with the bug only ever showing on other libcs.
I ran the full CI matrix on a fork first. The new test passes on every leg that runs the fuzzer: Linux x86-64 and 32-bit (-m32), macOS, valgrind, Cygwin, and MSVC x64 and Win32.
COVER_strict_cmp() and COVER_strict_cmp8() break ties between equal dmers by sample position but returned 1 when the two positions were equal, so cmp(a, a) was 1. A qsort() comparator must be consistent, and that includes comparing an element with itself. Suffix positions are unique, so the case is only reachable on a self-comparison and the change does not affect output: dictionaries are byte-identical before and after across 72 corpus/k/d combinations. Add a fuzzer test for the ordering itself. It trains COVER on low-entropy samples, where ties are frequent, and checks the dictionary hash against the one position ordering produces, for both comparator paths. In-place qsort() implementations order ties by partition state under the former address comparison, so the test fails against it (verified on macOS) and passes with position ordering.
Fixes #4185
Summary
stableSort()in the COVER dictionary builder is documented to leave each dmer group ordered by position in the input. It doesn't. The tie-break compares the addresses of the elements being sorted rather than the positions they hold, making the comparator depend on transient sort state instead of on the values being compared.As a result, dictionary output is not reproducible, it differs between C libraries, dmer frequencies are systematically under-counted, and on MSVC training degrades superlinearly — 458 s for a 16 MB corpus at
d=6, versus 6.5 s with this fix.The change is the same one-line edit at both tie-break sites, plus two one-line comment corrections. Four lines.
The bug
COVER_group()counts how many samples a dmer occurs in using a forward-only cursor, so it requires each dmer group to be ordered by position.COVER_ctx_init()states the assumption directly:/* The sort is stable, so each dmer group is sorted by position in input. */qsort()is not stable, soCOVER_strict_cmp()synthesises the ordering:lpandrpare the addresses of the elements being compared, not the positions they contain. Those coincide only whilesuffix[i] == i; the first swap breaks the correspondence, after which the tie-break orders by where an element currently sits rather than by what it holds. The comparator is therefore not a function of the values it compares.Consequences
1. Output depends on the sort implementation, and is not reproducible. Linking the unmodified builder against four different sorts, same input:
qsort24df76c6…012b539f…43b34fe5…0d85f425…A single implementation need not even be self-consistent between runs. When
qsort()compares an element against a temporary — a pivot copy — one operand is not in the array at all, so the comparison is stack-vs-heap and ASLR decides it. On Apple libc, the same binary on the same machine with the same input produced two distinct dictionaries across six consecutive runs at everydtested. Reproduced through the CLI and through a directly-linked single-threadedZDICT_trainFromBuffer_cover()call, so thread scheduling is not involved.Linux has been getting reproducible output, but by luck rather than design: glibc's
qsortuses mergesort whenever its internal allocation succeeds, and mergesort happens to satisfy the invariant. When that allocation fails, glibc falls back to heapsort, which does not — see Why accept a cost on glibc below.2. Frequencies are under-counted. 91% of tied groups emerge unordered, and 26–78% of dmer frequencies are under-counted — never over-counted, which is the signature of
COVER_group()'s forward-only cursor skipping occurrences.Why this is catastrophic on MSVC
The tie-break is only reached when two dmers compare equal, so the damage depends on how often that happens. Holding
d=8and the corpus at 8 MB, varying only content:With unique dmers the tie-break is never reached and there is no effect. This is why smaller
dlooks worse —dis a proxy for duplicate density, not a cause. Repetitive input is exactly what dictionary training targets.Measurements here are MSVC. #4185 also reports the hang under cygwin, msvcrt and ucrt with clang and gcc, which I have not measured — but the defect is a property of the comparator, not of any one implementation.
The fix
Compare the stored positions instead of the addresses. The key becomes
(dmer, position). Positions are unique, so no two elements compare equal, the order is total, and every conformingqsort()must produce the same arrangement — making stability unnecessary rather than merely unachieved.With this applied, all four sort implementations in the table above produce the identical dictionary
012b539f….The reporter of #4185 arrived at the same conclusion independently. Their C++ reimplementation was fast, and they noted in passing: "I had to change the 'tiebreaker' because the pointer value is not passed to the comparator."
Performance
Medians of 5–9 interleaved repetitions, each against a null control of identical binaries. Absolute times are comparable only within a row — MSVC and glibc share a generated 16 MB JSON corpus; Apple libc was measured on a separate 16 MB corpus on different hardware.
The effect tracks how quicksort-like each implementation is. glibc's mergesort performs the same comparisons either way, so it only pays for the extra dereference; quicksort-based implementations recover more than they pay.
At the parameters reported in #4185 (
d=10), MSVC:The speedup widens with corpus size because the current behaviour is superlinear and the fixed behaviour is not: n^1.52 versus n^1.13 at this
d. Extrapolating to the 100 MB dataset in the report, a single sort goes from roughly 15 minutes to about a minute; that run usedsteps=512, which performs many such sorts.The glibc cost is consistent and does not compound with corpus size (null ±0.33%):
Why accept a cost on glibc
glibc produces the correct ordering today, but not by design — only as a coincidence of its
qsort. That coincidence already fails on a reachable path.stdlib/qsort.cuses mergesort with a heapsort fallback when its internal allocation fails:Forcing that allocation to fail (same binary, same input, glibc 2.36):
e38748a6…e38748a6…03da4bc9…e38748a6…A dictionary trained under memory pressure silently differs from one trained normally. glibc also replaced this
qsortwith introsort and reverted it in January 2024; had that landed, Linux would have inherited the nondeterminism silently.Dictionary output
On glibc taking its mergesort path — the normal case — output is unchanged. Byte-identical across 215 configurations: 20 real corpora × 7 parameter sets on glibc 2.36, plus 5 synthetic content types × 3 sizes × 5 parameter sets on glibc 2.39. Corpus content was digest-verified before and after, and every dictionary hash reproduced in an independent second run.
glibc under memory pressure is the exception: today it falls back to heapsort and produces something different, as shown above. With this fix it produces the mergesort-path result regardless.
On Apple libc and MSVC dictionaries change to what glibc produces on its mergesort path, such that the dictionary produced from a given corpus is identical across runs and does not depend on the platform or architecture. Measured over 420 configurations spanning 20 corpora (source, English and Polish prose, executables, PCM audio, MRI/X-ray, JSON logs, database dumps): mean +0.52%, median +0.26% compression improvement; 271 better, 23 unchanged, 126 worse.
21 of 420 regress by more than 1%, with a median of −1.79%. Those concentrate in two corpora of very small records —
ghusers(7 cases, worst −13.9%) andplists(5 cases, worst −2.9%); outside those, no regression exceeds −2.4%. The baseline here was itself nondeterministic, which adds variance to individual comparisons but does not bias the mean.Verification
make checkwithDEBUGLEVEL=1;make test;make staticAnalyze(16 findings, identical to those on unmodifieddev— same count, checkers and locations, none incover.c); C90-Wall -Wextra -Werror -pedanticclean across fourDEBUGLEVEL/NDEBUGcombinations; deterministic across 6 consecutive runs; output verified identical on glibc/x86_64, MSVC/x86_64 and Apple libc/arm64.Validated on the full zstd CI suite: 103/103 checks pass, including nine QEMU architectures (ARM, ARM64, M68K, MIPS, PPC, PPC64LE, RISC-V, S390X, SPARC), eight Windows toolchain configurations, and the ASan/UBSan/MSan regression suites.