Skip to content

Improve genomeGenerate multicore index build performance#2687

Open
justinblethrow-cloud wants to merge 3 commits into
alexdobin:masterfrom
justinblethrow-cloud:indexbuild-opt-20260519
Open

Improve genomeGenerate multicore index build performance#2687
justinblethrow-cloud wants to merge 3 commits into
alexdobin:masterfrom
justinblethrow-cloud:indexbuild-opt-20260519

Conversation

@justinblethrow-cloud

@justinblethrow-cloud justinblethrow-cloud commented May 20, 2026

Copy link
Copy Markdown

Summary

  • improve genomeGenerate multi-core throughput for large references
  • batch suffix-array chunk filling by prefix bin, retain sorted chunks in RAM when memory allows, and split very large prefix bins into ordered sub-bins before sorting
  • parallelize SAindex traversal and junction-index work where output order can remain deterministic
  • add extras/tests/scripts/benchmarkGenomeGenerate.sh to capture wall time, CPU/I/O samples, and genomeGenerate stage timings

Benchmark notes

  • Local full CHM13v2 + ERCC + GTF benchmark, 96 threads, --genomeSAindexNbases 14, --genomeChrBinNbits 18, --limitGenomeGenerateRAM 300000000000
  • Baseline STAR 2.7.11b local run: 1164.89s
  • Best local run from this branch: 653.76s
  • Final pushed PR validation run: 685.29s in results/full_chm13_prfinal_20260520
  • In the final validation run, suffix-array chunk sorting took 282s wall; profiled internals were fill_count_seconds=1.629, fill_scatter_seconds=36.699, sort_seconds=237.783, finalize_seconds=1.788
  • Adaptive large-bin splitting sub-binned 116 large prefixes into 7427 sort ranges covering 194,932,600 suffixes
  • Outputs were byte-identical for Genome, SA, SAindex, chromosome metadata, and sjdbList.out.tab against the prior accepted optimized build

Validation

  • make -C source STAR
  • smoke benchmarkGenomeGenerate.sh runs at 1 and 16 threads
  • full CHM13v2 + ERCC + GTF benchmark with STAR_PROFILE_SA_SORT=1
  • byte comparisons for core generated index outputs

Notes for review

  • The benchmark harness is intentionally isolated under extras/tests/scripts/.
  • Optional suffix-sort profiling is gated behind STAR_PROFILE_SA_SORT=1; normal runs do not emit those profiling details.
  • The implementation keeps deterministic ordering for equal suffixes and was checked with byte comparisons on full-reference outputs.

Codex added 2 commits May 20, 2026 15:20
Add a local benchmark script for genomeGenerate that records wall time, CPU and I/O samples, selected index-build settings, and per-stage timings from Log.out. This keeps performance validation reproducible while leaving STAR runtime behavior unchanged.
Use parallel traversal for SAindex generation and parallel bucket sorting for junction insertion indices. Keep deterministic output ordering while reducing the remaining annotation-heavy genomeGenerate stages.
@justinblethrow-cloud

Copy link
Copy Markdown
Author

Cleaned the branch history for review. The PR now has three logical commits: benchmark harness, SAindex/junction parallelization, and suffix-array construction improvements. The cleaned tree is identical to the previously pushed tree (verified by matching git tree hashes), so the benchmark and byte-identity results in the PR description still apply. Local validation after cleanup: bash -n extras/tests/scripts/benchmarkGenomeGenerate.sh, git diff --check, and make -C source STAR.

Reduce genomeGenerate suffix-array build time by batching prefix-bin fills, retaining sorted chunks in RAM when memory allows, splitting very large prefix bins into ordered sub-bins, and using a comparator that can skip already-known prefix words. Optional SA sort profiling remains gated behind STAR_PROFILE_SA_SORT=1.
@justinblethrow-cloud

Copy link
Copy Markdown
Author

Update after final hardening pass:

  • Force-pushed cleaned 3-commit history at 47e6051.
  • Added a narrow guard around the adaptive sub-bin prefix lookup so it only uses funSAsortPrefixAtOffset when the extra prefix bases are available.
  • Rebuilt with make -C source STAR.
  • Re-ran full CHM13v2 + ERCC + GTF genomeGenerate validation at 96 threads: 685.29s wall in results/full_chm13_prfinal_20260520.
  • Byte-compared Genome, SA, SAindex, chromosome metadata, and sjdbList.out.tab against the prior accepted optimized output: all identical.

One broader skip-first-word safety guard was tested but not kept because it regressed suffix-sort time from the good ~238s profiled sort regime back to ~275s; the genomeGenerate buffer already has leading padding, and the retained guard is limited to the extra sub-bin prefix read.

@justinblethrow-cloud
justinblethrow-cloud marked this pull request as ready for review May 20, 2026 16:03
birdingman0626 added a commit to birdingman0626/STAR-Cross that referenced this pull request Jun 29, 2026
… 2.0)

MSVC's OpenMP 2.0 requires signed integral index variables in `#pragma omp for`.
The alexdobin#2687 ports used `uint` (64-bit) indices; switch the new parallel-for loops
to int64 (genomeGenerate genome scans, genomeSAindex chunks, sjdbBuildIndex
bucket count/scatter). No behavior change on GCC/Clang.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
birdingman0626 added a commit to birdingman0626/STAR-Cross that referenced this pull request Jun 29, 2026
…2687), WASP-in-SAM (alexdobin#2617), CodeQL fix (#8)

* chore(release): STAR-Cross 0.0.1 versioning, main branch, CodeQL third-party exclusion

- Set project/binary version to "STAR-Cross 0.0.1_<hash>" (CMakeLists + VERSION
  fallback); update version_check test regex to 0.0.1.
- Release workflow: name releases "STAR-Cross", auto-tag fallback 0.0.1_<sha>.
- CI branch triggers master -> main (codeql.yml).
- CodeQL: mark cpp-httplib include as SYSTEM (CodeQL skips system headers — the
  reliable fix for the cpp/non-https-url alert in the #include'd httplib.h, which
  paths-ignore cannot filter for C/C++); broaden config globs to **/_deps/**.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat: allow WASP vW:i tag in SAM output (upstream PR alexdobin#2617)

Emit the vW WASP filtering tag in the SAM/CRAM path and drop the BAM-only
restriction on --waspOutputMode and the vW attribute. Emission is scoped to
ATTR_vW only (the upstream patch incorrectly shared it with vG/vA).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* perf: multicore genomeGenerate suffix-array build (upstream PR alexdobin#2687)

Port the parallel prefix-bucketed suffix-array chunk sort (with sub-binning,
optional in-memory chunk retention, and a skip-first-word comparator fast-path)
from upstream PR alexdobin#2687. Reconciled with the fork:

- funCompareSuffixesFromWord uses the big-endian-safe loadUintLE loads.
- sjdbSortBucket reformulated without __uint128 (MSVC has no native 128-bit);
  the bucket mapping stays monotonic in the key, so the total order — and thus
  the final index — is unchanged.
- SA chunk packing keeps the in-memory path and the binary-mode disk fallback.

Index output is byte-identical to the previous builder; a new CI job
(validate-genome-index) builds the main baseline and this branch and diffs the
SA/SAindex/Genome across 1 vs 16 threads and a low-RAM multi-chunk layout.
build.yml CI branch triggers also move master -> main.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs: STAR-Cross 0.0.1, CRAM/big-endian/ARM, genomeGenerate perf, WASP-in-SAM

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(win): use signed loop indices in OpenMP parallel-for (MSVC OpenMP 2.0)

MSVC's OpenMP 2.0 requires signed integral index variables in `#pragma omp for`.
The alexdobin#2687 ports used `uint` (64-bit) indices; switch the new parallel-for loops
to int64 (genomeGenerate genome scans, genomeSAindex chunks, sjdbBuildIndex
bucket count/scatter). No behavior change on GCC/Clang.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
birdingman0626 added a commit to birdingman0626/STAR-Cross that referenced this pull request Jun 29, 2026
…am PR alexdobin#2687)

byteOrder.h routes genome/SA/packed-array access through loadUintLE/storeUintLE;
little-endian keeps the native single-instruction load, only big-endian compiles
take the portable path. Port alexdobin#2687's parallel prefix-bucketed SA build, reconciled
with the byte-order-safe comparator and MSVC (no __uint128; signed OpenMP indices).
Index output is byte-identical (verified in CI).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
birdingman0626 added a commit to birdingman0626/STAR-Cross that referenced this pull request Jun 29, 2026
…rk scripts

doctest coverage for the little-endian byte helpers (exercises the big-endian
algorithm on the LE runner) and a script that proves the alexdobin#2687 SA build yields a
byte-identical index vs baseline across thread counts and chunk layouts.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
birdingman0626 added a commit to birdingman0626/STAR-Cross that referenced this pull request Jun 29, 2026
… (upstream alexdobin#2657) (#10)

Root cause: in sjdbBuildIndex()'s SAi-insertion loop, the inner while increments
iSJ and then calls funCalcSAi(Gsj + indArray[2*iSJ+1], iL) WITHOUT re-checking
iSJ < nInd. When the scan advances past the last junction, iSJ reaches nInd and
reads the sentinel offset indArray[2*nInd+1] == (uint64)-999, i.e.
funCalcSAi(Gsj - 999, ...) — an out-of-bounds read ~999 bytes before the Gsj
allocation. On most heap layouts that is harmless garbage (the loop then exits
via the sentinel key), but where Gsj-999 is unmapped it segfaults. This matches
the report exactly: indArray[2*iSJ+1] == 18446744073709550617 == (uint64)-999.

Fix: break out of the loop when iSJ reaches nInd, before dereferencing the
sentinel. This yields the loop's intended "no matching junction" outcome (the
else branch resets iSJ), matching the sibling loop below which already guards
funCalcSAi with iSJ<nInd. Behavior is unchanged for runs that never reach the
sentinel, and it also removes the original's latent risk of funCalcSAi(garbage)
matching and writing a sentinel-based index.

Adds an AddressSanitizer CI job (asan-sjdb) exercising the sjdb-insertion path,
giving layout-independent coverage of this read (and of the alexdobin#2687 sort rewrite).

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant