Repository navigation
feat: use wyhash for minhashes - #347
Merged
Merged
Conversation
Member
|
Documentation preview available at https://docs.seqan.de/preview/seqan/hibf/347 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #347 +/- ##
=======================================
Coverage 99.76% 99.76%
=======================================
Files 53 54 +1
Lines 2154 2155 +1
Branches 8 8
=======================================
+ Hits 2149 2150 +1
Misses 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
eseiler
self-requested a review
October 26, 2026 12:33
seqan::hibf::sketch::wyhash is declared in hibf/sketch/wyhash.hpp and defined in
src/sketch/wyhash.cpp instead of being an inline function local to hyperloglog.cpp.
Adds the add_many benchmark to test/performance/sketch/hyperloglog_benchmark.cpp: each iteration
adds 2^20 pseudo-random uint64_t values (mt19937_64, seed 42, generated outside the timed region)
and reports items per second.
Benchmark against the previous commit (same compiler g++-16, Release, identical flags incl.
-O3 -flto=auto -march=native, HIBF_LTO_BUILD=ON):
- The timed loop of add_many is identical in both binaries up to addresses and the operand order
of one cmp. With LTO, add and wyhash are fully inlined (mulx, xor, shrx/shlx, lzcnt, cmovb);
there is no call.
- Timings (5 repetitions, interleaved, pinned to one core; the machine was loaded, so throughput
switched between ~430 M/s and ~670 M/s regardless of binary). Medians of the first round:
bits before after
5 427.6 M/s 426.7 M/s
8 431.2 M/s 429.7 M/s
12 428.4 M/s 430.0 M/s
No measurable difference. Not measured: HIBF_LTO_BUILD=OFF, where the out-of-line wyhash likely
costs a call per add().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Define seqan::hibf::sketch::wyhash as a constexpr function in hibf/sketch/wyhash.hpp and remove src/sketch/wyhash.cpp. The function is three instructions; inlining it no longer depends on LTO (HIBF_LTO_BUILD=OFF would otherwise cost a call per hyperloglog::add and per pushed minhash value). __uint128_t, unlike `unsigned __int128`, passes -pedantic -Werror with GCC 14/16 and Clang 20/23. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The header was copied from hyperloglog.hpp. wyhash was written for hibf in c0408ba (replacing xxHash) and is not derived from Ohno's MIT-licensed HyperLogLog, so the file is BSD-3-Clause only, as src/sketch/wyhash.cpp was. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move the links from the file documentation to the function documentation and note that wyhash(0) == 0, what this does to the sketches, and how a constant XOR would avoid it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the smallest 1000 hashes did not fill every minhash sketch, the refill pushed all kmers into the previous heap again, so the larger heap held its values twice and fill_incomplete_sketches appended duplicates. The refill now starts from an empty heap. Also: - With fewer than 1000 kmers, the heap kept max() placeholders, which filled sketch 15 with max() >> 4 instead of throwing. The heap is now at most as large as the set of kmers. - The heap grew to 2 * heap_size or threw if that exceeded the number of kmers, even though fewer hashes might have sufficed. It now grows to at most the number of kmers and throws only when all kmers did not suffice. - The uniqueness assert in fill_incomplete_sketches compared `hash` with the stored `hash >> 4`. Random user bins (uniform 64-bit values) were affected in about 1 % of cases for 3,000 to 100,000 kmers (duplicates) and 13 of 2000 for 1500 kmers (spurious exception). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reverts the version bump of 0b34275. Nothing checks the version on loading, so bumping it would suggest handling that does not exist. Archives written before the wyhash mixing still load; their values are not mixed and not comparable with newly computed sketches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`#if __AVX512F__ && __AVX512BW__` warns with -Wundef when AVX-512 is not enabled, which is an error with -Werror for every user of a public header. GCC 14/16, Clang 23 and icpx define both macros as 1 if the feature is enabled and leave them undefined otherwise (also with -mno-avx512f), so `defined(...)` is equivalent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With HIBF_NATIVE_BUILD=OFF, this would have caught the AVX-512 check in platform.hpp (19 translation units). The option is PRIVATE, so tests and other targets are not affected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
eseiler
approved these changes
Oct 26, 2026
eseiler
left a comment
eseiler
left a comment
Member
There was a problem hiding this comment.
Claude found some bugs that were not introduced in this PR, but I let them fix it.
eseiler
enabled auto-merge
October 26, 2026 16:57
eseiler
disabled auto-merge
October 26, 2026 16:57
eseiler
enabled auto-merge
October 26, 2026 16:58
eseiler
disabled auto-merge
October 26, 2026 17:03
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.
Solves #346