Repository navigation
Conversation
Replace the per-base unpack loop in l2b_getseq() with a head/middle/tail scheme: scalar bases until 4-base (byte) aligned, then a lookup table that emits 4 unpacked bases per step, then a scalar tail. The table maps each packed byte (four 2-bit bases, base 0 in the low bits) to its four unpacked bases as one little-endian uint32, written with memcpy so an unaligned seq buffer stays defined behavior. The table is filled once at startup via a constructor, so there is no per-call setup to amortize -- which is what sank an earlier word-wise attempt on the short (~162-base) seeding windows. Portable C, no SIMD and no arch guard, so it helps x86-64 and arm64 alike. Output is byte-identical to the scalar loop by construction; verified SAM- identical over full WGS (short-read) and HiFi (long-read) runs. Measured on an idle M3 Ultra at >=30s per arm: +1.1% short-read wall (reproduced across reps), long-read neutral.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
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
l2b_getseq()unpacks a run of 2-bit-packed reference bases into a byte-per-base buffer. Stock unpacks one base per iteration with a shift-and-mask (l2b_get0). This adds a 256-entry lookup table that maps a whole packed byte (4 bases) to its four unpacked bases at once, so the middle of the range is unpacked 4 bases per step with a single aligneduint32store, falling back to the scalar path for the unaligned head and tail.The table is a compile-time
static const uint32_t[256]— no runtime constructor. Each entry is laid out so that a little-endianuint32store emits the four bases in ascending position order; that byte-order assumption is enforced with a compile-time#errorguard rather than left to silently produce wrong output on a big-endian host. The table is the bulk-unpack equivalent ofl2b_get0(the canonical per-base accessor), cross-referenced in a comment as the packing source of truth.Correctness — byte-identical to stock (the invariant)
This is an
output = identicalchange. Verified byte-for-byte identical SAM to stockmasteron hg38 (GRCh38 + ALT/decoy), order-independent comparison, across short-read WGS and long-read HiFi, on multiple independent runs. Also byte-identical on x86 (below). The#errorguard makes the little-endian precondition a build-time failure, not a runtime correctness risk, on any non-LE target.Performance
arm64 (Apple M3 Ultra): short-read WGS wall neutral-to-slightly-positive (~0–1% across runs; earlier low-load measurement was +1.1%), long-read neutral (the table's benefit is proportional to the bulk-unpacked middle run). Precise wall is noise-limited under host load (large-index page-cache variance drives the reference base ~10% run-to-run); the byte-identity gate is the invariant.
x86 (AMD EPYC, AVX2, c6a): byte-identical and non-regressive (~+1.3% short-read WGS).
Review
The little-endian layout assumption is now enforced at compile time (
#errorguard) instead of relying on host byte order; the table is a compile-time constant with no__attribute__((constructor))runtime initializer; and it carries a comment tying it tol2b_get0as the packing definition. A remaining efficiency nitpick (reloading the packed word per iteration) was reviewed and left as-is — the flagged rewrite was not sound.Notes
Draft, review-only. Downstream-distribution feature vehicle, not a merge target; installation is via the
features.tomlmanifest, tracked separately.