dictBuilder: break COVER sort ties by position, not by address

[jaypatrickhoward]

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, so COVER_strict_cmp() synthesises the ordering:

result=lp<rp ? -1 : 1;

lp and rp are the addresses of the elements being compared, not the positions they contain. Those coincide only while suffix[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:

sort	dictionary (d=8)
libc qsort	24df76c6…
mergesort	012b539f…
quicksort	43b34fe5…
heapsort	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 every d tested. Reproduced through the CLI and through a directly-linked single-threaded ZDICT_trainFromBuffer_cover() call, so thread scheduling is not involved.

Linux has been getting reproducible output, but by luck rather than design: glibc's qsort uses 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=8 and the corpus at 8 MB, varying only content:

corpus	mean dmer group	current	with fix
random bytes	1.0 (all unique)	4.6 s	4.5 s
JSON-like records	104.5	45.0 s	3.6 s
two-symbol alphabet	6,400.0	150.4 s	2.3 s

With unique dmers the tie-break is never reached and there is no effect. This is why smaller d looks worse — d is 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 conforming qsort() 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.

platform	d	current	with fix	speedup
MSVC x86_64	6	458.39 s	6.52 s	70.3×
	8	133.43 s	6.54 s	20.4×
	16	20.96 s	12.19 s	1.72×
Apple libc arm64	6	3.697 s	3.417 s	1.082×
	8	3.757 s	3.515 s	1.069×
	16	4.993 s	4.654 s	1.073×
glibc x86_64	6	4.60 s	4.68 s	0.983×
	8	4.81 s	4.90 s	0.982×
	16	7.44 s	7.54 s	0.987×

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:

corpus	current	with fix	speedup
8 MB	20.2 s	4.1 s	4.9×
16 MB	53.9 s	7.9 s	6.8×
32 MB	166.7 s	19.8 s	8.4×

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 used steps=512, which performs many such sorts.

The glibc cost is consistent and does not compound with corpus size (null ±0.33%):

corpus	current	with fix	cost
4 MB	1.251 s	1.280 s	+2.31%
16 MB	5.160 s	5.289 s	+2.50%
64 MB	22.436 s	22.860 s	+1.89%

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.c uses mergesort with a heapsort fallback when its internal allocation fails:

if (!qsort_r_malloc (pbase, total_elems, size, cmp, arg, total_size))
/* Fallback to heapsort in case of memory failure. */heapsort_r (pbase, total_elems-1, size, cmp, arg);

Forcing that allocation to fail (same binary, same input, glibc 2.36):

	current	with fix
mergesort	e38748a6…	e38748a6…
heapsort fallback	03da4bc9…	e38748a6…

A dictionary trained under memory pressure silently differs from one trained normally. glibc also replaced this qsort with 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%) and plists (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 check with DEBUGLEVEL=1; make test; make staticAnalyze (16 findings, identical to those on unmodified dev — same count, checkers and locations, none in cover.c); C90 -Wall -Wextra -Werror -pedantic clean across four DEBUGLEVEL/NDEBUG combinations; 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.