diff --git a/CLAUDE.md b/CLAUDE.md index ee45985c..6cac7c1a 100755 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -100,4 +100,4 @@ Shared crates (`du-domain`, `du-atproto`, `du-bio`) live in the sibling repo `.. ### Useful Environment Variables -`NAVIGATOR_ANALYSIS_THREADS`, `NAVIGATOR_BGZF_THREADS`, `NAVIGATOR_IO_SYNC_MB` (how much a multi-GB writer may leave dirty in the page cache; `0` disables the pacing), `NAVIGATOR_Y_TREE_PROVIDER` (`decodingus`/`ftdna`), `NAVIGATOR_TREE_TTL_DAYS`, `NAVIGATOR_REFGENOME_DIR`, `NAVIGATOR_TREE_DIR`, `NAVIGATOR_ANCESTRY_PANEL` / `NAVIGATOR_ANCESTRY_PCA`, `DECODINGUS_APPVIEW_URL`. +`NAVIGATOR_ANALYSIS_THREADS`, `NAVIGATOR_BGZF_THREADS`, `NAVIGATOR_IO_SYNC_MB` (how much a multi-GB writer may leave dirty in the page cache; `0` disables the pacing), `NAVIGATOR_SORT_MB` / `NAVIGATOR_REVERT_SORT_MB` (spill-buffer size in MB; both are sized from installed RAM when unset), `NAVIGATOR_Y_TREE_PROVIDER` (`decodingus`/`ftdna`), `NAVIGATOR_TREE_TTL_DAYS`, `NAVIGATOR_REFGENOME_DIR`, `NAVIGATOR_TREE_DIR`, `NAVIGATOR_ANCESTRY_PANEL` / `NAVIGATOR_ANCESTRY_PCA`, `DECODINGUS_APPVIEW_URL`. diff --git a/crates/navigator-analysis/src/postprocess/sort.rs b/crates/navigator-analysis/src/postprocess/sort.rs index f4f8787e..506409df 100644 --- a/crates/navigator-analysis/src/postprocess/sort.rs +++ b/crates/navigator-analysis/src/postprocess/sort.rs @@ -1,8 +1,13 @@ //! Coordinate-sort a BAM, on disk. //! //! Same shape as [`crate::revert::collate`] and for the same reason: a WGS alignment does not fit -//! in memory, so this fills a fixed budget, sorts it, spills a run, and k-way merges the runs at -//! the end. Peak memory is the budget plus one buffered block per run, independent of input size. +//! in memory, so this fills a budget, sorts it, spills a run, and k-way merges the runs at the end. +//! Peak memory is the budget plus one buffered block per run, independent of input size. +//! +//! The budget comes from the machine ([`navigator_resource::spill_budget`]), not from a constant. +//! It was 512 MB for everyone, which on a 30x WGS spilled **688 runs** that the merge then opened +//! at once — bounded memory by design, and a great deal of fan-in to buy on a machine with 128 GB +//! sitting idle. //! //! ## Runs are ordinary BAM files //! @@ -35,9 +40,6 @@ use crate::error::AnalysisError; /// How often the record loop asks whether it has been cancelled — same cadence as the walkers. const CANCEL_CHECK_INTERVAL: u64 = 4096; -/// Default in-memory budget before a run is spilled. -const DEFAULT_SORT_BUFFER_MB: usize = 512; - /// Tuning for [`sort_alignment`]. #[derive(Debug, Clone)] pub struct SortParams { @@ -46,14 +48,12 @@ pub struct SortParams { } impl Default for SortParams { + /// Sized from the machine, not from a constant — see [`navigator_resource::spill_budget`], which + /// also documents `NAVIGATOR_SORT_MB`. The constant this replaced was 512 MB, which spilled 688 + /// runs on a 30x WGS regardless of whether the machine had 8 GB or 128 GB to work with. fn default() -> Self { - let mb = std::env::var("NAVIGATOR_SORT_MB") - .ok() - .and_then(|s| s.parse::().ok()) - .unwrap_or(DEFAULT_SORT_BUFFER_MB) - .max(1); Self { - buffer_bytes: mb * 1024 * 1024, + buffer_bytes: navigator_resource::spill_budget("NAVIGATOR_SORT_MB") as usize, } } } @@ -322,17 +322,38 @@ fn sort_key(record: &RecordBuf) -> (u32, u64) { } } -/// Rough heap footprint, for the memory budget. The variable-length parts dominate; the constant -/// covers the record's fixed fields and allocator overhead closely enough to size a buffer by. -fn heap_bytes(record: &RecordBuf) -> usize { +/// What one record costs the buffer. +/// +/// This used to be the variable-length parts plus a flat 256, which read as a reasonable stand-in +/// for "fixed fields and allocator overhead" and was not one. It left out the tag dictionary +/// entirely, and a mapped record carries a dozen tags — `NM`, `MD`, `AS`, `ms`, `nn`, `tp`, `cm`, +/// `s1`, `s2`, `de`, `rl` from minimap2 alone — each an entry in a `Vec<(Tag, Value)>`. The buffer +/// therefore held well over its stated budget, which mattered little against a constant picked with +/// an unwritten margin and matters a great deal now that the budget is a fraction of the machine +/// (see [`navigator_resource::spill_budget`]). +/// +/// Still an estimate: it does not chase a `Value`'s own heap (a string tag's bytes) or a `Vec`'s +/// spare capacity. It is close enough to size a buffer by, and it no longer omits a whole field. +pub(super) fn heap_bytes(record: &RecordBuf) -> usize { use noodles::sam::alignment::record::Cigar as _; - record.name().map(|n| n.len()).unwrap_or(0) + // The record itself sits inline in the buffer's `Vec`, so its size is part of what a record + // costs — not something to approximate around. + std::mem::size_of::() + + record.name().map(|n| n.len()).unwrap_or(0) + record.sequence().len() + record.quality_scores().len() + record.cigar().len() * 4 - + 256 + + record.data().len() * TAG_ENTRY_BYTES + // Name, sequence, qualities, CIGAR, tags: five vectors, five allocations. + + 5 * navigator_resource::ALLOCATION_OVERHEAD } +/// Bytes one tag occupies in a record's `Vec<(Tag, Value)>`. +/// +/// Pinned by `a_tag_entry_is_not_larger_than_the_estimate_assumes`, so a noodles upgrade that grows +/// `Value` fails a test here rather than quietly halving the buffer's honesty. +pub(super) const TAG_ENTRY_BYTES: usize = 48; + /// Stamp `@HD SO:coordinate` on the header. /// /// Not cosmetic: an index is only valid for a coordinate-sorted file, and readers decide whether diff --git a/crates/navigator-analysis/src/postprocess/tests.rs b/crates/navigator-analysis/src/postprocess/tests.rs index 6e2c5ec9..5763dedf 100644 --- a/crates/navigator-analysis/src/postprocess/tests.rs +++ b/crates/navigator-analysis/src/postprocess/tests.rs @@ -116,6 +116,51 @@ fn unsorted_fixture(dir: &Path) -> (PathBuf, sam::Header, usize) { (input, hdr, total) } +// ---- the buffer estimate --------------------------------------------------- + +/// The buffer is now sized as a fraction of the machine rather than a hand-picked constant, so what +/// a record is charged against it has to be roughly true. It was not: the tag dictionary was free, +/// and a mapped record carries a dozen tags. +#[test] +fn the_buffer_estimate_counts_the_tag_dictionary() { + use noodles::sam::alignment::record::data::field::Tag; + use noodles::sam::alignment::record_buf::data::field::Value; + + let bare = record("r0", Some(0), 1); + let mut tagged = bare.clone(); + for (tag, value) in [ + (Tag::ALIGNMENT_HIT_COUNT, Value::from(1i32)), + (Tag::MISMATCHED_POSITIONS, Value::from("10")), + (Tag::ALIGNMENT_SCORE, Value::from(60i32)), + ] { + tagged.data_mut().insert(tag, value); + } + + let charged = heap_bytes(&tagged) - heap_bytes(&bare); + assert_eq!(charged, 3 * TAG_ENTRY_BYTES, "three tags should cost three entries"); +} + +/// A noodles upgrade that grows `Value` should fail here, rather than quietly making every buffer +/// hold more than its budget says. +#[test] +fn a_tag_entry_is_not_larger_than_the_estimate_assumes() { + use noodles::sam::alignment::record::data::field::Tag; + use noodles::sam::alignment::record_buf::data::field::Value; + + assert!( + std::mem::size_of::<(Tag, Value)>() <= TAG_ENTRY_BYTES, + "a tag entry is {} bytes, which the estimate does not cover", + std::mem::size_of::<(Tag, Value)>() + ); +} + +/// The record's own size is part of what it costs — it lives inline in the buffer's `Vec`. +#[test] +fn the_buffer_estimate_covers_the_record_itself() { + let empty = RecordBuf::default(); + assert!(heap_bytes(&empty) >= std::mem::size_of::()); +} + // ---- the properties that matter ------------------------------------------- /// Coordinate order, with unplaced reads at the end where SAM puts them. diff --git a/crates/navigator-analysis/src/revert/collate.rs b/crates/navigator-analysis/src/revert/collate.rs index b68f648b..38640bee 100644 --- a/crates/navigator-analysis/src/revert/collate.rs +++ b/crates/navigator-analysis/src/revert/collate.rs @@ -8,7 +8,8 @@ //! //! The property that matters is that peak memory is the budget plus one buffered block per run, //! *independent of input size*. That is what lets the same code path revert a 5 GB exome and a -//! 200 GB WGS on the same laptop, and it is why this is disk-backed rather than clever. +//! 200 GB WGS on the same laptop, and it is why this is disk-backed rather than clever. The budget +//! itself is sized from the machine — see [`navigator_resource::spill_budget`]. //! //! Runs use a plain length-prefixed binary encoding rather than a serialization framework: the //! format is written and read in this one file, it is a hot path measured in billions of records, @@ -25,7 +26,8 @@ use super::transform::{Mate, RevertedRead}; use crate::error::AnalysisError; /// Buffer size for run spill/read-back. Large enough that the merge's per-run reads stay -/// sequential, small enough that a few dozen concurrent runs don't add up to real memory. +/// sequential, small enough that the runs a WGS produces don't add up to real memory when the merge +/// holds all of them open at once. const RUN_IO_BUFFER: usize = 256 * 1024; /// Accumulates reverted reads, spilling sorted runs to scratch when the budget is reached. diff --git a/crates/navigator-analysis/src/revert/mod.rs b/crates/navigator-analysis/src/revert/mod.rs index 84da79ee..607be822 100644 --- a/crates/navigator-analysis/src/revert/mod.rs +++ b/crates/navigator-analysis/src/revert/mod.rs @@ -56,11 +56,6 @@ pub use transform::{Mate, RevertedRead}; /// reasoning as the other walkers (see [`crate::cancel`]). const CANCEL_CHECK_INTERVAL: u64 = 4096; -/// Default memory budget for the sort buffer before a run is spilled to scratch. 256 MiB holds -/// roughly a million 150 bp reads with names, which keeps the run count (and so the merge's open -/// file handles) low for a WGS input without being greedy on an 8 GB machine. -const DEFAULT_SORT_BUFFER_MB: usize = 256; - /// What to do with a **primary** record whose CIGAR contains a hard clip. /// /// Hard clipping means the aligner discarded sequence from the record, so the read cannot be fully @@ -90,14 +85,14 @@ pub struct RevertParams { } impl Default for RevertParams { + /// The collator is sized from the machine by the same rule as the coordinate sort — see + /// [`navigator_resource::spill_budget`], which also documents `NAVIGATOR_REVERT_SORT_MB`. The + /// constant this replaced was 256 MB, described as keeping the run count low for a WGS; at + /// ~340 bytes a reverted read that is a run every million reads, so a 30x WGS spilled several + /// hundred of them and the merge opened every one. fn default() -> Self { - let mb = std::env::var("NAVIGATOR_REVERT_SORT_MB") - .ok() - .and_then(|s| s.parse::().ok()) - .unwrap_or(DEFAULT_SORT_BUFFER_MB) - .max(1); Self { - sort_buffer_bytes: mb * 1024 * 1024, + sort_buffer_bytes: navigator_resource::spill_budget("NAVIGATOR_REVERT_SORT_MB") as usize, hard_clipped: HardClipPolicy::default(), prefer_original_qualities: true, } diff --git a/crates/navigator-analysis/src/revert/transform.rs b/crates/navigator-analysis/src/revert/transform.rs index 55dfbd04..27dfb274 100644 --- a/crates/navigator-analysis/src/revert/transform.rs +++ b/crates/navigator-analysis/src/revert/transform.rs @@ -64,7 +64,15 @@ impl RevertedRead { /// Rough heap footprint, for the sort's memory budget. The two `Vec`s dominate; the constant /// covers the struct itself and allocator overhead closely enough to size a buffer by. pub fn heap_bytes(&self) -> usize { - self.name.len() + self.sequence.len() + self.qualities.len() + 64 + // The read sits inline in the collator's `Vec`, so its own size counts; the three vectors + // each cost an allocation on top of their contents. The flat 64 this replaced was smaller + // than the struct alone, which was harmless against a fixed 256 MB budget and is not + // against a budget sized from the machine (see `navigator_resource::spill_budget`). + std::mem::size_of::() + + self.name.len() + + self.sequence.len() + + self.qualities.len() + + 3 * navigator_resource::ALLOCATION_OVERHEAD } } diff --git a/crates/navigator-app/src/realign_job.rs b/crates/navigator-app/src/realign_job.rs index 9cd34c23..649ca7aa 100644 --- a/crates/navigator-app/src/realign_job.rs +++ b/crates/navigator-app/src/realign_job.rs @@ -356,8 +356,10 @@ impl App { let dir = scratch.path().join("revert"); clear_stage_dir(&dir); let token = cancel.clone(); + let params = RevertParams::default(); + log_buffer("collating", params.sort_buffer_bytes); let stats = tokio::task::spawn_blocking(move || { - revert::revert_alignment(&bam, reference.as_deref(), &dir, &RevertParams::default(), &token) + revert::revert_alignment(&bam, reference.as_deref(), &dir, ¶ms, &token) }) .await .map_err(|e| AppError::Join(e.to_string()))??; @@ -466,9 +468,11 @@ impl App { // weight — and at WGS scale they are tens of GB the sort is about to want back. clear_stage_dir(&dir); let token = cancel.clone(); + let params = SortParams::default(); + log_buffer("sorting", params.buffer_bytes); stage(&sorted, async { tokio::task::spawn_blocking(move || { - postprocess::sort_alignment(&input, &out, &dir, &SortParams::default(), &token, &mut |_| {}) + postprocess::sort_alignment(&input, &out, &dir, ¶ms, &token, &mut |_| {}) }) .await .map_err(|e| AppError::Join(e.to_string()))? @@ -673,6 +677,15 @@ fn gb(bytes: u64) -> u64 { bytes / 1_000_000_000 } +/// Record the spill budget a stage is about to run with. +/// +/// Both budgets now come from the machine rather than a constant, so the run count they produce is +/// no longer inferable from the version number. Without this line, "it spilled 400 runs" in a bug +/// report is a fact with nothing to attach it to. +fn log_buffer(stage: &str, bytes: usize) { + eprintln!("realign: {stage} with a {} MB buffer", bytes / (1024 * 1024)); +} + /// Whether a job needing `needed` bytes may start given `free` bytes available. /// /// Split from the syscall so the *decision* is testable on any machine; the probe itself is not. diff --git a/crates/navigator-resource/src/lib.rs b/crates/navigator-resource/src/lib.rs index 479035a9..055b1c7e 100644 --- a/crates/navigator-resource/src/lib.rs +++ b/crates/navigator-resource/src/lib.rs @@ -150,6 +150,75 @@ impl Write for PacedFile { } } +/// The budget an external-sort stage holds in memory before spilling a run to disk. +/// +/// Both of the pipeline's spill-to-disk stages — the revert's collator and the coordinate sort — +/// were sized by a constant, 256 MB and 512 MB, chosen when nobody had run a WGS through them. +/// The measured cost of that: a 30x WGS coordinate sort spilled **688 runs**, all of which the +/// merge then opens at once. It is bounded memory by design and it does work, but on a machine with +/// 128 GB of RAM it is a lot of fan-in bought for no reason, and the constant that produced it was +/// the same on a laptop that genuinely needed it. +/// +/// So the number comes from the machine. `var` still wins when it is set — an explicit MB count is +/// the escape hatch for a run that has to be reproduced or squeezed — and the sizing is otherwise: +/// +/// - **A quarter of installed RAM.** Total rather than free, because free fluctuates with whatever +/// the user happens to have open, and a stage whose run count depends on the browser is a stage +/// whose behaviour cannot be reproduced from a bug report. +/// - **Never below 512 MB.** That is the sort's existing default, so no machine sorts with less +/// than it does today. +/// - **Never above 8 GB.** Past that the returns are gone — 88 runs against 44 is nothing next to +/// 688 against 88 — while the costs are not: the stable sort allocates half the buffer again as +/// scratch, and growing the record vector doubles its allocation while still holding the old one. +/// - **Never more than half of what is free right now.** The stable part of the rule assumes a +/// machine that is otherwise idle. When it is not, spilling an extra run is cheap and swapping +/// the buffer is not. +/// +/// Memory the platform will not report reads as zero, and zero means *unknown*, which must not be +/// read as "no memory" — the same rule [`classify`] follows. An unknown machine gets the floor. +pub fn spill_budget(var: &str) -> u64 { + if let Some(mb) = std::env::var(var).ok().and_then(|s| s.parse::().ok()) { + return mb.max(1) * 1024 * 1024; + } + let mut system = sysinfo::System::new(); + system.refresh_memory(); + budget(system.total_memory(), system.available_memory()) +} + +/// What one heap allocation costs beyond the bytes asked for: the allocator's own bookkeeping plus +/// rounding up to a size class. +/// +/// It lives beside [`spill_budget`] because the two are one contract. A budget is only as honest as +/// the tally that fills it, and a stage that counts only payload bytes will hold well over its +/// budget in real memory — which is fine against a hand-picked 512 MB constant chosen with a margin +/// nobody wrote down, and not fine against a fraction of the machine. +/// +/// Sixteen bytes is the conventional figure for the allocators on the three desktop targets. This +/// is a budget estimate rather than an audit; the point is that a record with four small vectors +/// costs meaningfully more than the sum of their lengths. +pub const ALLOCATION_OVERHEAD: usize = 16; + +/// The floor, and the answer for a machine that will not say how much memory it has. +const MIN_SPILL_BUDGET: u64 = 512 << 20; +/// The ceiling. See [`spill_budget`] for why bigger stops paying. +const MAX_SPILL_BUDGET: u64 = 8 << 30; + +/// The sizing decision, split from the probe so it is testable on any machine — the same split +/// [`classify`] makes, and for the same reason. +fn budget(total_memory: u64, available_memory: u64) -> u64 { + if total_memory == 0 { + return MIN_SPILL_BUDGET; + } + let budget = (total_memory / 4).clamp(MIN_SPILL_BUDGET, MAX_SPILL_BUDGET); + if available_memory == 0 { + return budget; + } + // The floor holds even here: a machine this short of memory would have taken 512 MB under the + // old constant anyway, so honouring the busy-machine guard past that point would be a + // regression dressed as caution. + budget.min((available_memory / 2).max(MIN_SPILL_BUDGET)) +} + /// How hard the machine is being leaned on. /// /// Bands, not a single threshold, because the interesting reading is the trend: a stage that @@ -361,6 +430,58 @@ mod tests { assert_eq!(classify(0, 0, 0), Pressure::Normal); } + /// The case the autosizing exists for: a big machine should stop spilling hundreds of runs. + #[test] + fn a_large_machine_gets_the_ceiling() { + assert_eq!(budget(128 << 30, 100 << 30), MAX_SPILL_BUDGET); + } + + #[test] + fn an_ordinary_machine_gets_a_quarter_of_it() { + assert_eq!(budget(16 << 30, 12 << 30), 4 << 30); + } + + /// A machine whose memory is already spoken for gets a smaller buffer, because an extra spilled + /// run costs a file and swapping the buffer costs the run. + #[test] + fn a_busy_machine_is_held_to_half_of_what_is_free() { + assert_eq!(budget(64 << 30, 6 << 30), 3 << 30); + } + + /// Never below what the sort used before any of this existed. + #[test] + fn a_small_or_busy_machine_never_goes_under_the_old_default() { + assert_eq!(budget(2 << 30, 2 << 30), MIN_SPILL_BUDGET); + assert_eq!(budget(64 << 30, 100 << 20), MIN_SPILL_BUDGET); + } + + /// Unknown is not zero. A platform that will not report memory must not be sized as if it had + /// none — and must not be sized as if it had plenty either. + #[test] + fn unknown_memory_gets_the_floor() { + assert_eq!(budget(0, 0), MIN_SPILL_BUDGET); + assert_eq!(budget(64 << 30, 0), MAX_SPILL_BUDGET.min(16 << 30)); + } + + /// The escape hatch has to win, or a run cannot be reproduced on a different machine. + #[test] + fn an_explicit_override_beats_the_machine() { + let var = "NAVIGATOR_TEST_SPILL_MB_OVERRIDE"; + std::env::set_var(var, "7"); + assert_eq!(spill_budget(var), 7 * 1024 * 1024); + std::env::remove_var(var); + } + + /// A nonsense override must not produce a zero-byte budget, which would spill one run per + /// record. + #[test] + fn a_zero_override_is_floored_at_one_megabyte() { + let var = "NAVIGATOR_TEST_SPILL_MB_ZERO"; + std::env::set_var(var, "0"); + assert_eq!(spill_budget(var), 1024 * 1024); + std::env::remove_var(var); + } + #[test] fn written_bytes_accumulate() { let before = bytes_written(); diff --git a/documents/design/realignment-module.md b/documents/design/realignment-module.md index c08ce6dd..48b0b1eb 100644 --- a/documents/design/realignment-module.md +++ b/documents/design/realignment-module.md @@ -640,10 +640,27 @@ as a page-cache promise — which matters most for `mapped.bam`, since its BGZF precisely what a resumed run reads to decide whether it can trust the file, and the .mmi index, whose atomic rename otherwise publishes contents the disk has not acknowledged. -The sort buffer is worth revisiting separately: at the default 512 MB it spilled **688 runs**, which -the merge then opens at once. That is bounded memory by design and it works, but on a 128 GB machine -it is a lot of fan-in bought for no reason. `NAVIGATOR_SORT_MB` already exists; sizing its default -from installed RAM is not yet done. +**The spill budgets now come from the machine.** At the fixed 512 MB the sort spilled **688 runs**, +which the merge opens at once — bounded memory by design, and a lot of fan-in to buy on a 128 GB +machine that was otherwise idle. The revert's collator had the same shape at 256 MB. Both now call +`navigator_resource::spill_budget`: a quarter of installed RAM, never below the old 512 MB default, +never above 8 GB, and never more than half of what is free at the moment the stage starts. +`NAVIGATOR_SORT_MB` and `NAVIGATOR_REVERT_SORT_MB` still override it, and the job logs the figure it +chose so a run's spill count stays explicable afterwards. + +Total RAM rather than free RAM for the stable part of the rule, deliberately: a stage whose run +count depends on how many browser tabs were open is a stage whose behaviour cannot be reproduced +from a bug report. The ceiling is there because the returns stop — 88 runs against 44 is nothing +next to 688 against 88 — while the costs do not: the stable sort allocates half the buffer again as +scratch, and growing the record vector holds two allocations at once. + +Sizing from the machine also required the tally to be honest, which it was not. The sort charged a +record for its name, sequence, qualities and CIGAR and **not for its tag dictionary** — a dozen tags +on every minimap2 record, at 40 bytes an entry — so a "512 MB" buffer held closer to a gigabyte. An +unwritten margin inside a hand-picked constant is harmless; the same margin inside a fraction of +installed RAM is how a machine ends up swapping. Both estimators now count the record's own size, +its tag dictionary, and per-allocation overhead, and a test pins the tag-entry figure so a noodles +upgrade that grows `Value` fails there rather than silently. ## Phase 5 result — WGS229 end to end (2026-08-14)