Repository navigation
Reduce native Windows CRAM extraction overhead - #163
Merged
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
| Layer / File(s) | Summary |
|---|---|
Bounded CRAM stop decoding packaging/windows/recipes/{bcftools,htslib,samtools}/patches/*bounded-cram-stop-copy.patch, packaging/windows/tests/cram_stop.c |
The three decoder patches replace byte-by-byte scans with bounded searches and update the decoder’s output and input positions. The test compares the real decoder with a scalar reference across varied inputs. |
SSSE3 BAM sequence packing packaging/windows/recipes/{bcftools,htslib,samtools}/patches/*vector-bam-sequence-packing.patch, packaging/windows/tests/bam_seq.c |
The patches add guarded vector packing for eligible 16-byte chunks and retain scalar handling for other chunks. The test checks packed bases, qualities, and odd-length sequences across varied inputs. |
CRAM buffer reuse packaging/windows/recipes/{bcftools,htslib,samtools}/patches/*reuse-cram-buffers.patch, packaging/windows/tests/cram_buffers.c |
The patches add platform-gated CRAM buffer pools and use them in allocation, release, and replacement paths. The test covers concurrent compression and decompression, boundary sizes, decode recovery, and sanitizer probes. |
Windows build and test integration packaging/windows/README.md, packaging/windows/recipes/env.sh, packaging/windows/recipes/{bcftools,htslib,samtools}/build.sh, packaging/windows/recipes/{bcftools,htslib,samtools}/recipe.yaml |
Build scripts pass gcc-ar. The shared HTSlib build adds LTO and inline memory-operation flags and runs the three tests. Recipes apply the patches and increment build numbers; the README documents the changes. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Refactor
Merge Risk: 
52cc7
-
This change reduces Windows CRAM decoding overhead through bounded copying, SSSE3 sequence packing and reusable buffers, with tests for each path. No actionable merge-blocking risk was found. The remaining CI, production-package and full-genome checks listed in the PR description are normal pre-merge validation.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files. (13 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary objective: reducing native Windows CRAM extraction overhead through buffer reuse and decoding optimizations. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files. (13 skipped: 13 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks 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 @coderabbitai help to get the list of available commands.
Owner
Author
|
@coderabbitai full review |
✅ Action performed
|
There was a problem hiding this comment.
🔵 Needs a closer look
Native memory-management and sequence-encoding changes need human sign-off after CI, installed-package validation, and the remaining large SNP/FASTQ checks.
No open issues 🎉
What changed in this PR
This PR reduces native Windows CRAM extraction overhead in the HTS tools used by the CLI, while preserving the existing CPU requirement and floating-point behavior.
Changes:
- Adds bounded CRAM buffer reuse, bulk copying of stop-terminated fields, and SSSE3 BAM sequence packing across all three HTS recipes.
- Enables HTS-specific link-time optimization and inline memory operations, and increments package build numbers.
- Adds build-time equivalence and concurrent buffer tests, with documentation of optimization limits.
| File | Description |
|---|---|
| packaging/windows/tests/cram_stop.c | Checks stop-codec equivalence against scalar behavior. |
| packaging/windows/tests/cram_buffers.c | Tests concurrent buffer use, growth, and error recovery. |
| packaging/windows/tests/bam_seq.c | Checks sequence packing against scalar lookup results. |
| packaging/windows/recipes/samtools/recipe.yaml | Registers optimization patches and increments the build number. |
| packaging/windows/recipes/samtools/patches/0004-reuse-cram-buffers.patch | Adds bounded buffer reuse to bundled HTSlib. |
| packaging/windows/recipes/samtools/patches/0003-vector-bam-sequence-packing.patch | Adds SSSE3 sequence packing with scalar fallback. |
| packaging/windows/recipes/samtools/patches/0002-bounded-cram-stop-copy.patch | Adds bounded stop-field scanning and bulk copying. |
| packaging/windows/recipes/samtools/build.sh | Uses the LTO-aware archiver and adds ShellCheck annotations. |
| packaging/windows/recipes/htslib/recipe.yaml | Registers optimization patches and increments the build number. |
| packaging/windows/recipes/htslib/patches/0003-reuse-cram-buffers.patch | Adds bounded CRAM buffer reuse. |
| packaging/windows/recipes/htslib/patches/0002-vector-bam-sequence-packing.patch | Adds SSSE3 sequence packing with scalar fallback. |
| packaging/windows/recipes/htslib/patches/0001-bounded-cram-stop-copy.patch | Adds bounded stop-field scanning and bulk copying. |
| packaging/windows/recipes/htslib/build.sh | Uses the LTO-aware archiver and adds ShellCheck annotations. |
| packaging/windows/recipes/env.sh | Applies HTS optimization flags and runs native test harnesses. |
| packaging/windows/recipes/bcftools/recipe.yaml | Registers optimization patches and increments the build number. |
| packaging/windows/recipes/bcftools/patches/0003-reuse-cram-buffers.patch | Adds bounded buffer reuse to bundled HTSlib. |
| packaging/windows/recipes/bcftools/patches/0002-vector-bam-sequence-packing.patch | Adds SSSE3 sequence packing with scalar fallback. |
| packaging/windows/recipes/bcftools/patches/0001-bounded-cram-stop-copy.patch | Adds bounded stop-field scanning and bulk copying. |
| packaging/windows/recipes/bcftools/build.sh | Uses the LTO-aware archiver and adds ShellCheck annotations. |
| packaging/windows/README.md | Documents optimizations, cache limits, and build-time checks. |
Review effort: Balanced
Merged
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.
Native Windows HTS extraction spends substantial CPU time decoding CRAM and grouping single-sample pileups. This PR adds bounded CRAM buffer reuse, bounded bulk copying for stop-terminated fields, SSSE3 sequence packing with scalar equivalence, and a single-sample
bcftools mpileupgrouping copy. Multiple inputs or samples keep the upstream grouping path. The changes target native Windows x86-64-v2 static UCRT packages without changing floating-point flags.The cache retains at most 16 MiB, 16 entries, two entries per size class, and accepts reuse only within 12.5% excess capacity. HTS recipes use link-time optimization and inline memory operations. The bcftools recipe advances to
wgse_3; htslib remainswgse_2and samtools remainswgse_3. The build installs MSYS2 GNUpatchexplicitly for the grouping oracle.Validation completed:
mpileup.c. The run skipped publication. The four commits added here produce a tree byte-for-byte identical to that tested candidate, including the patch; the measured diagnostic patch has the same SHA-256.git diff --checkpass on the integrated head. The exact candidate tree also passed source mypy and the fresh sanitizer run; current-head CI is running.Open gates before merge/release: install the selected
.condaartifacts through Pixi in an isolated native Windows prefix and verify package/executable hashes; compare small and full-genome outputs; finish matched Windows VM versus Linux VM cohorts on the same SATA SSD and require Windows elapsed time <=120% of Linux; complete current-head CI and review. The candidate packages are not published or pinned. PR remains open pending these gates.