Repository navigation
dictBuilder: speed up the COVER dmer sort with a multi-key quicksort - #4928
Open
jaypatrickhoward wants to merge 2 commits into
Open
jaypatrickhoward wants to merge 2 commits into
jaypatrickhoward wants to merge 2 commits into
Conversation
stableSort() reached qsort() through a five-variant selection block (ZDICT_QSORT_APPLE/GNU/MSVC/C11/C90) that picked between qsort_r(), qsort_s() and a non-reentrant global, plus the _GNU_SOURCE and __STDC_WANT_LIB_EXT1__ feature tests needed to expose them. Sort the dmers directly instead, with a multi-key quicksort (Bentley & Sedgewick): partition on a single dmer byte and recurse into the equal bucket one byte deeper. A byte proven equal is never examined again, which matters here because dmers are heavily duplicated and a comparison sort restarts every comparison at byte 0. The order produced is the same total order as before -- by dmer, then by position -- so dictionaries are unchanged. Byte order follows each comparator: COVER_cmp8() compares a masked little-endian U64, making byte d-1 most significant, while COVER_cmp() uses memcmp(), making byte 0 most significant. MEM_readLE64() byte-swaps on big-endian hosts, so this holds on every architecture. Recursion descends into the two smaller spans and loops on the largest. If L is the largest span and A is either other, A <= L and A + L <= n, so A <= n/2 and depth is bounded by log2(n) regardless of d or input shape. Measured maximum depth is 18 across corpora designed to stress it. The sort is in-place and allocates nothing, the comparison is inlined rather than reached through a function pointer, and the platform dispatch and its global are removed. With no qsort_r() left to detect, the ZDICT_QSORT build macro goes too: drop its entry from lib/README.md, and its override from the musl CI job, whose musl 1.2.2 has no qsort_r() and now builds without the override. Extend the existing COVER ordering test to d=6, where dmers shorter than 8 bytes are compared as a masked 8-byte word; that path had no test. Its expected hash comes from the qsort()-based code this replaces.
zstd_deps.h defined _GNU_SOURCE on Linux, Cygwin and MSYS solely to expose qsort_r() to the dictionary builder, as its comment said. compiler.h defined WIN_CDECL for the comparators the dictionary builder passed to MSVC's qsort_s(). The COVER sort no longer calls either, nothing else uses them, and an internal header should not opt every file that includes it into GNU extensions. Cleanup only: the previous commit does not depend on this one.
This branch has not been deployed
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.
What and why
stableSort()reachesqsort()through a five-variant selection block (ZDICT_QSORT_APPLE/GNU/MSVC/C11/C90) choosing betweenqsort_r(),qsort_s()and a non-reentrant global, plus the_GNU_SOURCEand__STDC_WANT_LIB_EXT1__feature tests needed to expose them.This replaces that with a multi-key quicksort over the dmers. Dictionaries are byte-identical everywhere: this is a pure performance and simplification change.
qsortis quicksort-based (macOS, Windows) and 1.5–2.0× with glibc's mergesort; with musl's smoothsort it is 4–14× fasterkanddis 1.3–2.2× faster end to end on macOS, Windows and FreeBSD, 1.02–1.4× on glibc, and 2.8–7.0× on musl; the default parameter search, where tryingkvalues dominates, gains 0–34%.text(+0.15%), allocates nothing, recursion depth provably ≤ log₂(n)cover.cZDICT_trainFromBuffer_cover()thread-safe on every platform (ZDICT_trainFromBuffer_cover is not thread safe #4045), and makes the open PRs Enable qsort_r() on more OS's #4468 and dictBuilder: declare qsort_r on OpenHarmony (musl headers omit it) #4886 unnecessaryChecklist
qsort_r()detection to more platforms; this PR removes the detection instead, which makes both unnecessary (see Portability).qsort_r(), not the goal. For cover.c does not build on OpenHarmony: qsort_r is exported but not declared (musl) #4872 and Does not build with musl libc -- qsort_r not available there #4311, themusl-buildCI job, with its override removed, fails ondevwith the same undeclared-qsort_r()error and passes here. For ZDICT_trainFromBuffer_cover is not thread safe #4045, see the ThreadSanitizer reproducer under Validation.Tools used
Claude Code, under my direction and review, for writing the code and the test, and for benchmarking.
Why a different algorithm
The dmers being sorted are fixed-length byte strings that share long prefixes, and a comparison sort restarts every comparison at byte 0. Quicksort compares many elements against one fixed pivot, so deep in the recursion it re-walks the prefix those elements share with that pivot, repeatedly.
A multi-key quicksort (Bentley & Sedgewick) partitions on a single dmer byte and recurses into the equal bucket one byte deeper, so a byte already proven equal is never examined again.
Alternatives considered
Ten algorithms were benchmarked against the same workload before settling on this one. Measurements below come from a standalone harness reproducing the same comparator and data, so they are indicative rather than the shipping numbers quoted later.
qsortat smalldbut collapse to ~0.5× at larged. They restart each comparison at byte 0, so they re-walk shared prefixes — the same defect that makes the current code slow, not a fix for it.qsortat anyd(0.44–0.80×). Ruled out immediately.d=112640from stack exhaustion. Making it safe required a depth guard that falls back to multi-key quicksort anyway.d) with no early exit — 0.04× atd=4096.Three refinements to the chosen algorithm were also tested and rejected: 16-bit digits instead of 8 (4–12% slower — a wider digit shrinks the equal bucket, which is where this algorithm earns its advantage), an "already ascending" pre-check before the position sort (neutral), and an all-equal fast path (neutral, and subsumed by the loop-on-largest recursion described under Safety).
Dictionaries are unchanged
Byte-identical to
devin 252 of 252 configurations: 6 platforms × 7dvalues (6, 8, 12, 16, 32, 1024, 112640) × 6 corpus sizes (1, 4, 8, 12, 16, 32 MB).The test added in #4765, which pins the exact dictionary bytes, passes. This PR also extends it from
d= 8 and 16 tod= 6, the path for dmers shorter than 8 bytes; the new expected hash comes fromdev'sqsort-based code.The end-to-end runs below add 70 more identical results, including musl, FreeBSD and MinGW-w64.
Also identical under QEMU on 9 architectures (ARM, ARM64, PPC, PPC64LE, S390X, MIPS, RISC-V, M68K, SPARC) at
d= 6, 8, 9 and 16: 36 of 36, covering five big-endian targets (PPC, S390X, M68K, SPARC, MIPS) and both 32- and 64-bit word sizes. Every target produced not merely a matching dictionary but the same one.Byte order follows each comparator:
COVER_cmp8()compares a masked little-endian U64, making byted-1most significant, whileCOVER_cmp()usesmemcmp(), making byte 0 most significant.MEM_readLE64()byte-swaps on big-endian hosts, so this holds on every architecture.Performance
How it was measured
dev(6e51a24) and this PR were built side by side on each machine, by the project's own build system with its default release flags, and run alternately on the same data. Every machine is a GitHub-hosted runner except one, marked local.ubuntu-24.04-armubuntu-latestmacos-latestmacos-15-intelwindows-11-armwindows-latestwindows-latestubuntu-latestubuntu-24.04-armubuntu-24.04Each runner has 4 vCPUs, except macOS arm64 with 5. The Linux glibc, macOS x86-64 and Windows MSVC rows ran both the sort-phase and the end-to-end benchmarks. The macOS arm64 runner ran the sort-phase benchmark only: GitHub had no macOS arm64 runner available for the end-to-end job, so that job ran on my laptop instead, with the same script, commits and timer patch. The last four rows ran end to end only.
make zstdwith the Makefile's defaults (-O3, plus its warning and feature flags) for gcc and clang;cmake --config Release(/O2 /Ob2 /MD) for MSVC. Each job logs the exact command line that compiledcover.c.githubregression dataset (500 JSON files, 408 KB).clock()aroundstableSort(), inserted by a benchmark-only patch applied identically to both builds; end to end is whole-process wall time.bench-e2ebranch of my fork, at ab29a0feb; the job logs, with the exact compiler command lines, are in runs 43083049529 (sort) and 43083056295 (end to end).The sort
Sort-phase time only. Each platform has 42
(corpus size, d)configurations, each the median of 3 runs; the columns are the worst, median and best of those 42.Across all 252 configurations (6 platforms × 7
dvalues × 6 corpus sizes) the minimum is 1.05×. There is no platform, corpus size ordvalue where the sort is slower.Restricted to the parameters most users actually hit (
d∈ {6, 8, 12, 16}, corpora 1–16 MB), per-platform medians are 1.31× / 1.75× / 2.36× / 2.35× / 2.66× / 4.19× in the row order above.The spread tracks the quality of each platform's
qsort. glibc's mergesort handles this workload well, so the gain there is smallest. Apple's libc (whoseqsortcomes from FreeBSD) and MSVC are quicksort-based and pay the repeated-prefix cost, as does FreeBSD itself; musl's smoothsort pays it most of all (both end to end, below).End to end
Whole-process
zstd --train-covertime. The fixed-k/dcolumn covers four synthetic cases (8 and 32 MB,d= 8 and 16,k= 2048) plus thegithubdataset (k= 1024,d= 8, 16 KB dictionary); the parameter search is the default--train-coveron the 8 MB corpus and ongithub.k/d, end to enddev's runtimeWith fixed parameters the sort is most of the work, so most of its gain reaches the user. The parameter search trains once per candidate
kbut sorts only once perd, so the sort is a small share of its runtime and the end-to-end gain is correspondingly small.Safety
Recursion depth is bounded by log₂(n), independent of
dor input. The recursion descends into the two smaller spans and loops on the largest; ifLis the largest span andAeither other, thenA ≤ LandA + L ≤ n, soA ≤ n/2.Measured maximum depth across
d∈ {6, 8, 16, 1024}, on 4 MB corpora chosen to stress it unless noted:abrepeatedDepth 1 on the all-identical corpus is the loop-on-largest rule at work: the equal bucket is the whole range, so it is looped rather than recursed. The random two-symbol corpus is the deepest because every byte splits each range roughly in half, so recursion continues until ranges reach the insertion-sort cutoff: log₂(4 M / 24) ≈ 17.4.
Stack frame size, read from the function prologue on each ABI:
COVER_MAX_SAMPLES_SIZEcaps n at ~4.29 G, so depth ≤ log₂(4.29 G) ≈ 32. Worst case is therefore 32 frames × 200 B (the largest frame measured) ≈ 6.4 KB of stack; the deepest measured, 18 frames, is ≈ 3.6 KB. The sort allocates nothing.Code size
gcc 13.3 on x86-64 (Ubuntu 24.04),
make -C libwith its defaults, and withCFLAGS=-Osfor the last row:devlibzstd.a, strippedlibzstd.so.textcover.o.textcover.o.textat-OsBuilds without
ZSTD_LIB_DICTBUILDERare unaffected. Thelibzstd-sizeCI check is unaffected too: it limits the file size oflibzstd.so, and that file is 96 bytes smaller (913,544 → 913,448, same compiler); its other three configurations are the same size or smaller.Portability and thread safety
The dispatch exists because
qsort_ris not portable, and that has caused repeated problems:call to undeclared function 'qsort_r'.qsort_r, so zstd 1.5.7 fails to link there. musl cannot be detected from the preprocessor, so__linux__ && !__ANDROID__still selects theqsort_rpath, and affected users must setZDICT_QSORT=ZDICT_QSORT_C90by hand. Themusl-buildCI job does exactly that.qsort_r()but its headers do not declare it, socover.cfails to compile:call to undeclared function 'qsort_r'.ZDICT_trainFromBuffer_cover()is not thread-safe, because the C90 fallback keeps its context in a file-scope global.The C90 fallback is wider than it looks: it is where every platform the selection block does not name ends up, which includes FreeBSD and the other BSDs, MinGW-w64, Android, Solaris and musl builds with the override. The C11 branch would not rescue any of them, because it tests
STDC_LIB_EXT1rather than__STDC_LIB_EXT1__and so is never selected automatically.Two open PRs extend the detection further: #4468 enables
qsort_r()on more systems, and #4886 declares it by hand for OpenHarmony. With noqsort_r()call left, neither is needed.These are one root cause. Sorting the dmers directly removes it: no
qsort_r, nothing to detect, no manual override, and no global —ZDICT_trainFromBuffer_cover()becomes thread-safe on every platform. Themusl-buildjob (Ubuntu 22.04, musl 1.2.2) drops its override, and theZDICT_QSORTentry comes out oflib/README.md. Reproduced in an Ubuntu 22.04 container:devwithout the override fails there withimplicit declaration of function 'qsort_r', and this branch passes the job's exact command.Consistency and simplification
Sort behavior is now defined by zstd rather than inherited from whichever
qsortthe platform ships, so training speed no longer depends on the C library. On arm64 runners with the same CPU (Neoverse-N2), training on the 8 MB corpus atd=8 took 1.7 s with glibc and 6.4 s with musl before this change, and 1.6 s and 1.5 s after. The code also no longer needs to know which of fiveqsortspellings is available.Removed: the
ZDICT_QSORT_*selection block, theqsort_r/qsort_s/qsortdispatch, the non-reentrantg_coverCtxglobal, bothCOVER_strict_cmpwrappers, and the_GNU_SOURCE/__STDC_WANT_LIB_EXT1__feature tests — 42 lines of platform-conditional code, for a net of +29 lines incover.c.A second commit removes two leftovers of the
qsort_rdispatch from shared headers: the_GNU_SOURCEdefine inlib/common/zstd_deps.h, whose comment says it is there only to exposeqsort_r()to the dictionary builder, and theWIN_CDECLmacro inlib/common/compiler.h, which marked the comparators passed to MSVC'sqsort_s()and now has no users. This is cleanup only: dropping that commit gives up none of the speed, portability or thread-safety benefits above, so it can be left out if you would rather not touch those headers.Validation
make checkandmake testpass. A deliberately brokend< 8 comparison passes the test's original two cases but fails the newd= 6 case.cover.ccompiles cleanly as C90 with-pedantic -Werrorand the library's full warning set, at fourDEBUGLEVEL/NDEBUGsettings and at both-O0and-O3. ASan/UBSan are clean atd∈ {1, 6, 7, 8, 9, 15, 16, 17, 32, 1024, 112640}, withshrink, with a split point and on the multithreaded optimize path, and every dictionary matchesdev's. MemorySanitizer reports no uninitialized reads, running CI's three MSan jobs (fuzzer unit tests, fuzzing and the CLI tests) and COVER training at ninedvalues plus the parameter search.ThreadSanitizer, for #4045: eight threads training at once (
d=8 andd=16, each on its own data, three rounds) report no races, and every dictionary matches the same training run done alone. The same test againstdevbuilt for its C90 path (-DZDICT_QSORT=ZDICT_QSORT_C90) reports a race ong_coverCtxand gets 21 of 24 dictionaries wrong.make staticAnalyze(clang 23) reports no bugs, on this branch or ondev.