dictBuilder: break COVER sort ties by position, not by address - #1
Closed
jaypatrickhoward wants to merge 1 commit into
Closed
dictBuilder: break COVER sort ties by position, not by address#1jaypatrickhoward wants to merge 1 commit into
jaypatrickhoward wants to merge 1 commit 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.
jaypatrickhoward
commented
Sep 6, 2026
OwnerAuthor
CI dry run complete: 103/103 checks passed. Superseded by the upstream PR: facebook#4765 |
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
CI dry run on the fork, per CONTRIBUTING.md:
The full description will go on the upstream PR against
facebook:devonce this is green.Local checks already passing:
make check,make test,make staticAnalyze(16 findings, identical to unmodifieddev), C90-Wall -Wextra -Werror -pedanticacross four DEBUGLEVEL/NDEBUG combinations.