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.
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user