From 49e66f16a221ebc7cc90a53e0cba7a0c0fc72a31 Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Sat, 15 Aug 2026 20:15:43 +0200 Subject: [PATCH] docs: document architecture analysis and refactoring plans for siblings Documents architectural analysis, identified performance bottlenecks including hardcoded selection flags and redundant full-index scans causing I/O-bound stalls. Details planned pipeline refactoring to unify stages around a single shared selection for a fused single-pass scan, noting future dependencies on entropy-biased family selection. --- docmd/architecture/siblings.md | 66 ++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/docmd/architecture/siblings.md b/docmd/architecture/siblings.md index 2a7d26c6..a69a25ed 100644 --- a/docmd/architecture/siblings.md +++ b/docmd/architecture/siblings.md @@ -389,6 +389,72 @@ undermining the point. It stays plausible as a coarse filter for the predicts low true entropy too), but not as a stand-in for the high tail — not pursued further for now. +## `--free-loss`/`--tnt` pipeline: four independent scans, three of them unsampled (found 2026-08-15, not yet fixed) + +Measured on `phyloskims_sal_vac` (91 genomes): `obikmer phylo --subsample 500000 +--free-loss --tnt` logs four sequential stages — +`raw_snp_distance` (1413s), `base_pair_tally` (1456s), +`cardinality_tally` (2078s), `snp_pseudo_alignment` (143s). Reading the +code (`obikmer/src/cmd/phylo/mod.rs:210-242`, +`obikphylo/src/siblings/distance.rs`, `cardinality.rs`, `alignment.rs`) +surfaced two compounding problems, not one: + +1. **Four separate full scans of the annex**, each opening its own + `KmerPartition`/`PartitionCache` and calling `scan_family_pairs`/ + `scan_layer_families` independently — nothing computed in one stage is + reused by another. `base_pair_tally` is explicitly documented + (`distance.rs:138-143`) as a second full pass over the same + traversal `raw_snp_distance` already did, needed only because + `raw_snp_distance` doesn't keep the resolved bases, only aggregate + counts. `cardinality_tally` and `snp_pseudo_alignment` are each a + third and fourth independent full pass. Per the module's own earlier + profiling note (`family_scan.rs:26-28`, cited already above), this + traversal is page-fault/mmap-bound, not compute-bound — the ~10-12% + CPU efficiency ("contention" status) observed on these three slow + stages is consistent with I/O stalls scaled by repeated full scans, + not lock contention (there are no `Mutex`/`RwLock` anywhere in + `siblings/*.rs`; shared writes use per-slot `AtomicU8::fetch_or`). +2. **Worse: `raw_snp_distance` and `cardinality_tally` don't honor + `--subsample` at all** — both call `scan_layer_families` with + `Selection::All` hardcoded, and `args.subsample` isn't even threaded + into their function signatures (`mod.rs:212`, `mod.rs:226`). Only + `snp_pseudo_alignment(args.subsample)` builds a real reservoir-sampled + `Selection::Some(set)` (via `compute_selections`, `alignment.rs:89`). + So today, `--subsample 500000` only bounds the pseudo-alignment step — + the SNP-distance matrix, the Sankoff base-pair calibration, and the + cardinality histogram are always computed over the **full, unsampled** + index regardless of the flag. This is not merely "different subsamples + per stage" (which would already be a problem worth fixing) — it's that + three of the four stages never subsample, which explains most of the + ~10x runtime gap against `snp_pseudo_alignment` on its own. + +**Decided requirement (2026-08-15, not yet implemented)**: all four +stages must consume **one shared selection**, computed once, not each +stage either scanning everything or drawing its own independent sample. +Per-genome-pair SNP counts, the Sankoff base-pair calibration, the +cardinality histogram, and the pseudo-alignment must all describe the +same set of families — otherwise the calibration and the alignment it's +meant to calibrate aren't even guaranteed to agree on which sites exist. +Once selection is unified, doing four independent full scans stops making +sense on its own terms: a single pass over the shared selection can feed +all four accumulators (SNP/shared counts, base-pair tally, cardinality +tally, pseudo-alignment output) at once, which is also the fix for +problem 1 above — the two issues turn out to be one architectural +decision, not two. + +**Forward-looking complication, explicitly flagged**: the eventual plan +is to bias family *selection itself* by entropy15 (favor the informative +mid-entropy band over uniform reservoir sampling — the exact motivation +behind "Cheap entropy pre-filtering" above). That means selection can no +longer be an uninformed draw made *during* the single fused scan; entropy +(exact or a cheap proxy) has to be known *before* selection happens, +which argues for a first, cheap, entropy-informed selection pass followed +by one fused, unsampled-relative-to-that-selection scan — tying this +decision directly to the still-unresolved marginal-proxy limitation +described in "Cheap entropy pre-filtering" above (which only reliably +predicts the *low*-entropy tail, not the high one — a real gap, not +solved by this note, if entropy-biased selection ships before it is). + ## Two entropy definitions kept side by side, for comparison (2026-08-15) `--shannon`'s CSV carries both `entropy15` (`family_entropy` — the settled