Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions .github/workflows/check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,76 @@ jobs:
- name: Clippy check
run: cargo ci-lint

# Builds and tests the optional in-process bwa-mem3 aligner backend (feature
# `aligner-bwa-mem3`, off by default). This is the one feature that pulls a
# C++ toolchain into the build (the vendored bwa-mem3 kernels via
# `bwa-mem3-sys`'s `cc`-driven build.rs), so it gets its own job rather than
# folding into `test`/`lint`: those two stay C-toolchain-free, matching every
# other job's fast/portable assumption.
#
# Matrix covers both architectures the feature compiles on (the dependency is
# gated in Cargo.toml to `x86_64`/`aarch64` — see the `bwa-mem3-rs` entry).
# `ubuntu-24.04-arm` is a real (not emulated) arm64 GitHub-hosted runner, so
# this exercises the NEON build path, not just the x86_64 SIMD-tier ladder.
#
# Toolchain deps are deliberately minimal: `build-essential` (a C++17-capable
# g++) and `zlib1g-dev` (the crate links `z`) are everything `bwa-mem3-sys`'s
# build.rs actually needs for a normal build — mirrors fg-labs/bwa-mem3-rs's
# own `build`/`lint` jobs. Two toolchain pieces this job deliberately does
# NOT install:
# - `libclang-dev`: bindgen only runs under the sys crate's own
# `regenerate-bindings` feature (gated in its build.rs), which this job
# never enables — the committed bindings are used as-is.
# - clang-19 / libomp: bwa-mem3's Makefile compiler floor (clang >= 19 /
# gcc >= 15) applies to building the vendored bwa-mem3 CLI *from
# upstream* (fg-labs/bwa-mem3-rs's own `e2e` job does that, to diff
# against this crate's output byte-for-byte); `cc` enforces no such floor
# on the sys crate itself, and this job never builds the reference CLI
# binary.
aligner-ffi:
strategy:
fail-fast: false
matrix:
os: [ubuntu-24.04, ubuntu-24.04-arm]
runs-on: ${{ matrix.os }}
timeout-minutes: 30
steps:
- name: Checkout code
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
persist-credentials: false
- name: Install build deps (C++17 toolchain for bwa-mem3-sys)
run: sudo apt-get update && sudo apt-get install -y build-essential zlib1g-dev
- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8 # stable
with:
components: clippy
- name: Set up compilation cache (sccache)
uses: mozilla-actions/sccache-action@fc920bf0ec8de6ee65d409111f7ec508035751ba # v0.0.11
- name: Install nextest
uses: taiki-e/install-action@b20dedce73af6905cdc30d6611090c9b67557c8d # v2.85.12
with:
tool: nextest
- name: Unit tests (aligner-bwa-mem3)
run: cargo nextest run --features aligner-bwa-mem3 -p fgumi --locked
Comment thread
coderabbitai[bot] marked this conversation as resolved.
- name: Clippy (aligner-bwa-mem3, pedantic)
# Lints compare,simulate,profile-adjacency,aligner-bwa-mem3 together:
# the first three are already part of every default dev/test build's
# assumed surface (`cargo ci-lint` compiles them), so linting them
# together with the aligner feature catches any interaction between the
# two rather than linting the aligner feature in isolation.
run: |
cargo clippy --workspace --all-targets \
--features compare,simulate,profile-adjacency,aligner-bwa-mem3 --locked \
-- -D warnings -W clippy::pedantic
- name: Rustdoc (aligner-bwa-mem3, -D warnings)
# `cargo ci-doc` (the `docs` job) does not enable `aligner-bwa-mem3`, so
# the feature-gated in-process backend's docs are rendered only here.
# Clippy does not report broken intra-doc links; rustdoc does.
env:
RUSTDOCFLAGS: "-D warnings"
run: cargo doc --no-deps -p fgumi --document-private-items --features aligner-bwa-mem3 --locked

# `merge_slots.rs` swaps `std::sync` for `loom::sync` under `--cfg loom`, and
# `tests/loom_merge_slots.rs` is `#![cfg(loom)]`. Neither is built by any other
# job, so without this one a broken model — or a `cfg(loom)` build that stopped
Expand Down
29 changes: 25 additions & 4 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ cargo bench
- `compare` - Enable compare subcommand (developer tools)
- `simulate` - Enable simulate command for test data generation
- `profile-adjacency` - Enable profiling output for adjacency UMI assigner
- `aligner-bwa-mem3` - Opt-in in-process bwa-mem3 aligner backend for `runall` (cohort math, engine abstraction; the backend itself is still being wired in); off by default, and compiling it requires a C++17 toolchain (see `Cargo.toml`'s `bwa-mem3-rs` dependency)

Build with features: `cargo build --release --features compare,simulate`

Expand Down Expand Up @@ -133,15 +134,35 @@ Uses `mimalloc` as global allocator for performance.
`#[allow(unsafe_code)]` blocks are permitted only at the documented sites listed below;
any new `unsafe` block requires updating this section with a written justification.

Enabling the optional `aligner-bwa-mem3` feature links `bwa-mem3-sys`/`bwa-mem3-rs`
(vendored C++ plus its Rust FFI bindings), which contain their own `unsafe` outside
this workspace's `deny(unsafe_code)`; fgumi's own crates carry no new `unsafe` for it
and remain `#![deny(unsafe_code)]` regardless of the feature.

### Approved non-stdlib FFI exceptions

The following external FFI calls are approved because they back core infrastructure:

- **`libmimalloc_sys`** (`crates/fgumi-sort/src/memory_probe.rs`) — mimalloc is the configured
global allocator. The FFI wrappers (`force_mi_collect`, `process_rss_bytes`, and the
`memory-debug`-gated `print_mi_stats` calling `mi_stats_print_out`) are isolated to
a single `#[allow(unsafe_code)]` sub-module. mimalloc synchronizes these calls
internally, so they are safe to invoke concurrently.
global allocator. The FFI wrappers (`force_mi_collect`, `process_rss_bytes`, the
`memory-debug`-gated `print_mi_stats` calling `mi_stats_print_out`, and
`retain_freed_memory` / `mi_purge_delay_ms` calling `mi_option_set` /
`mi_option_get`) are isolated to a single `#[allow(unsafe_code)]` sub-module.
mimalloc synchronizes these calls internally, so they are safe to invoke
concurrently. `retain_freed_memory` sets mimalloc's purge delay to -1 (never
return freed pages to the OS); the align stage calls it at wiring (via
`retain_freed_memory_unless_user_set`) unless `MIMALLOC_PURGE_DELAY` is set,
because its per-batch buffer churn otherwise costs hundreds of thousands of page
faults. The setting is process-wide and never restored, so it also turns
fgumi-sort's `force_mi_collect()` into a no-op for later `runall` stages; measured
end to end (extract through consensus, 1M pairs, 32 threads) it is still ~4%
faster, sort spills included, for ~0.4 GB more peak RSS.
The subprocess route also passes `MIMALLOC_PURGE_DELAY=-1` to the aligner child
through its environment, which needs no FFI. A safe alternative does not exist:
mimalloc reads its environment only at process start, and `libmimalloc-sys`
exports no safe setter.
The option index (15) is not exported as a constant; a unit test pins it against
the linked mimalloc v3's 1000 ms default.
- **`mach2`** (`crates/fgumi-sort/src/memory_probe.rs`, macOS only) — `task_info(TASK_VM_INFO)`
is the only way to read `phys_footprint` (the RSS metric mimalloc reports accurately).
Isolated to the same sub-module as the mimalloc FFI.
Expand Down
22 changes: 22 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,14 @@ mach2 = { workspace = true }
[target.'cfg(target_os = "linux")'.dependencies]
nix = { workspace = true }

# In-process bwa-mem3 aligner backend (feature `aligner-bwa-mem3`). Gated on the
# only two architectures whose vendored C++ SIMD tiers build (`x86_64`/`aarch64`);
# enabling the feature elsewhere hits a `compile_error!` in
# `pipeline::steps::align`. 0.3.0 is the first
# release with the three-phase (`seed_extend` / `infer_cohort` / `pair_emit`) API.
[target.'cfg(any(target_arch = "x86_64", target_arch = "aarch64"))'.dependencies]
bwa-mem3-rs = { version = "0.3.0", optional = true }
Comment thread
coderabbitai[bot] marked this conversation as resolved.

[features]
default = ["consensus"]
# Single umbrella feature for all consensus calling (simplex + duplex + codec).
Expand Down Expand Up @@ -210,6 +218,19 @@ simulate = ["consensus"]
profile-adjacency = []
# Enable slow stress tests for concurrency testing
stress-tests = []
# In-process bwa-mem3 aligner backend for `fgumi runall` (its steps and the
# `bwa-mem3-inproc` preset build on this). Off by default: it pulls the
# optional `bwa-mem3-rs` dependency, which compiles the vendored bwa-mem3 C++ (C++17 toolchain
# required) and links it statically. Default builds stay pure Rust with no C
# toolchain. The in-process backend code is `#[cfg(feature = "aligner-bwa-mem3")]`
# confined to `pipeline::steps::align::inproc`.
#
# `mimalloc/override` makes the global mimalloc also interpose the C `malloc`/
# `free`, so the linked bwa-mem3 C++ uses mimalloc instead of the system
# allocator (whose per-thread arena contention regresses multithreaded
# alignment). Scoped to this feature because it is the only build that links the
# C++ aligner; a pure-Rust fgumi build gains nothing from the C interpose.
aligner-bwa-mem3 = ["dep:bwa-mem3-rs", "mimalloc/override"]
# Test-only feature (not part of the supported public API). Its former gated
# helper (the legacy multi-thread engine's `run_bam_pipeline_with_grouper`) was
# removed in R6/C6; retained as the self dev-dependency's feature hook. Enabled
Expand Down
4 changes: 4 additions & 0 deletions crates/fgumi-sort/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ pub use memory_probe::print_mi_stats;
/// pipeline thread-telemetry sampler, which needs an RSS reading on every tick
/// regardless of whether that feature is enabled.
pub use memory_probe::process_rss_bytes;
/// Tune mimalloc to keep freed pages instead of returning them to the OS, and
/// read the setting back. Re-exported for the align stage, which frees and
/// reallocates large per-batch buffers on every pool thread.
pub use memory_probe::{mi_purge_delay_ms, retain_freed_memory};
/// Background read-ahead record reader, re-exported for `fgumi compare bams`,
/// which reads two inputs concurrently and needs each decode off the main thread.
pub use read_ahead::RawReadAheadReader;
Expand Down
48 changes: 47 additions & 1 deletion crates/fgumi-sort/src/memory_probe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ use log::{Level, debug, log_enabled};
#[allow(unused_imports)]
// consumed by main fgumi via the crate-root re-export
pub use platform_ffi::print_mi_stats;
pub use platform_ffi::{force_mi_collect, process_rss_bytes};
pub use platform_ffi::{
force_mi_collect, mi_purge_delay_ms, process_rss_bytes, retain_freed_memory,
};

/// Log target for the memory probe.
///
Expand Down Expand Up @@ -73,6 +75,33 @@ mod platform_ffi {
}
}

/// mimalloc's `mi_option_purge_delay`. `libmimalloc-sys` does not export it
/// as a constant, so this is its position in `mi_option_t`, which is 15 in
/// both the v2 and v3 `mimalloc.h` enums (deprecated slots keep their
/// places). `mi_purge_delay_ms_matches_mimalloc_default` pins it against the
/// linked mimalloc v3's default of 1000 ms.
const MI_OPTION_PURGE_DELAY: libmimalloc_sys::mi_option_t = 15;

/// Stop mimalloc returning freed pages to the OS (`purge_delay = -1`), so a
/// page that is freed and soon reused is not decommitted and faulted back
/// in. Trades a higher peak RSS for fewer page faults and less system time.
pub fn retain_freed_memory() {
// SAFETY: mi_option_set only stores the option value in mimalloc's
// option table; mimalloc reads it on each purge, so setting it after
// start-up is supported, and the option index is fixed (see above).
unsafe {
libmimalloc_sys::mi_option_set(MI_OPTION_PURGE_DELAY, -1);
}
}

/// mimalloc's current purge delay in milliseconds (`-1`: never purge).
#[must_use]
pub fn mi_purge_delay_ms() -> i64 {
// SAFETY: mi_option_get only reads mimalloc's option table.
#[allow(clippy::useless_conversion)] // `c_long` is `i32` on Windows
i64::from(unsafe { libmimalloc_sys::mi_option_get(MI_OPTION_PURGE_DELAY) })
}

/// Print mimalloc allocator statistics to stderr via the default output handler.
///
/// `mi_stats_print_out(None, null_mut())` uses mimalloc's internal synchronization,
Expand Down Expand Up @@ -533,6 +562,23 @@ impl Default for MergeProbe {
mod tests {
use super::*;

/// Pins [`platform_ffi`]'s purge-delay option index: the linked mimalloc v3
/// defaults `mi_option_purge_delay` to 1000 ms (v2 used 10 ms), and retaining
/// freed memory sets it to -1. Skipped when the environment overrides it.
#[test]
fn mi_purge_delay_ms_matches_mimalloc_default() {
// mimalloc matches its option names case-insensitively.
if std::env::vars_os().any(|(name, _)| {
name.eq_ignore_ascii_case("MIMALLOC_PURGE_DELAY")
|| name.eq_ignore_ascii_case("MIMALLOC_RESET_DELAY")
}) {
return;
}
assert_eq!(mi_purge_delay_ms(), 1000);
retain_freed_memory();
assert_eq!(mi_purge_delay_ms(), -1);
}

#[test]
fn test_fmt_bytes_units() {
assert_eq!(fmt_bytes(0), "0 B");
Expand Down
Loading
Loading