Repository navigation
CI validation: #4765 review response (do not merge) - #4
Closed
jaypatrickhoward wants to merge 2 commits into
Closed
jaypatrickhoward wants to merge 2 commits into
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.
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.
Owner
Author
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.
Fork-only run of the full matrix for 8dbec80 (three-way COVER tie-break + golden-hash fuzzer test) before it goes on meta#4765. Do not merge.