Fix batch enumeration offsets and refactor sibling annex construction

Shifts sibling annex construction from slot-indexed enumeration to iteration-order traversal by correcting cumulative k-mer offset tracking in batch enumeration. Replaces coarse per-partition parallelism with chunked work distribution to prevent thread starvation on skewed partitions. Decouples custom progress messages from ETA updates to eliminate display clobbering during high-frequency callbacks. Adds regression tests validating batch offset correctness, partial batch handling, and iterator-order consistency across layer builds.
This commit is contained in:
Eric Coissac
2026-08-16 20:51:19 +02:00
parent d2548e8c33
commit 32d6720f50
11 changed files with 413 additions and 88 deletions
+98 -10
View File
@@ -18,7 +18,8 @@ method that reconstructs a kmer from a bare slot number is wrong by
construction, regardless of the mechanism used (MPHF re-hash, or evidence
decode + direct unitig read). See `MphfLayer::kmer_at`
(`obilayeredmap/src/mphf_layer.rs`) — flagged for removal, currently called
from `obikindex/siblings/build.rs:122` and `family_scan.rs:173`.
from `obikphylo/siblings/build.rs` and `family_scan.rs` (since removed — see
"Pending work" status below).
## Two pipelines, never mixed
@@ -39,7 +40,7 @@ pointless even in `Exact`/`Hybrid` mode since the kmer was already known.
The sibling annex (`FamilyMask`/`SiblingAnnex`, `.psib`,
`obicompactvec/src/siblingannex.rs`) records, per kmer, whether it is a
family minorant and which family members are present in the index. Its only
consumers (`obikindex/siblings/stats.rs`, `family_scan.rs`) enumerate it
consumers (`obikphylo/siblings/stats.rs`, `family_scan.rs`) enumerate it
exhaustively (`0..annex.len()`); no query-pipeline code path touches it.
**Decision**: the annex must be persisted in iteration order, not slot
@@ -66,12 +67,99 @@ partition is unknown) and must keep going through
`KmerPartition::query_partition_with` (MPHF + evidence), never a raw
`index()`.
## Pending work
## Pending work — done
- Remove `MphfLayer::kmer_at`.
- `siblings/build.rs`: build `slot_kmer`-equivalent via iteration, not
`kmer_at`; thread iteration index instead of slot through the
variant/reconciliation pipeline; persist the annex in iteration order.
- `siblings/family_scan.rs`, `stats.rs`: read the annex via zipped
iteration (`iter_kmers().zip(annex_iter)`) instead of `0..annex.len()` +
`kmer_at`.
The plan above shipped: `obikphylo` (a new crate — phylo-domain extension
traits over `obikindex::KmerIndex`/`obilayeredmap::Layer<D>`, replacing the
old `obikindex::siblings` module) builds and reads the annex purely in
iteration order (`SiblingLayerExt::iter_siblings`/`iter_minorants`, both with
batch variants, mirroring `Layer<D>`'s own `KmerIter`/`KmerBatchIter`
shape). `MphfLayer::kmer_at` has no remaining callers.
A separate, unrelated bug surfaced during this work and was fixed
(2026-08-14): `MphfLayer::enumerate_kmers_batch` computed its
`batch_start_index` via the stdlib `.enumerate()` adapter, which counts
*batches* (0, 1, 2…), not the cumulative k-mer offset the annex is actually
keyed on — every batch past the first wrote its mask/annex entries at the
wrong iteration-order position. Fixed by tracking a running offset instead;
regression tests added (`sibling_annex_no_empty_masks_after_build`,
`sibling_histogram_does_not_panic_on_partial_last_batch`).
## Performance: `build_sibling_annex` parallelism (2026-08-14)
Investigated on a real multi-genome run (`phyloskims_sal_vac`, k=31/m=11).
Baseline: mostly one active core, with short multi-core bursts — average
~3 cores.
**Fixes that helped, kept:**
- `CanonicalKmerOf::minimizer()` (`obikseq/src/kmer.rs`) — a direct O(k)
bit-arithmetic minimiser for a single isolated k-mer, replacing a
`RollingStat` instance fed byte-by-byte through an ASCII round-trip (used
by `helpers::partition_of`, called for every generated family variant).
~3x wall-clock improvement on its own, confirmed by sampling
(`obiskbuilder::rolling_stat`/`obikentropy` frames disappeared from the
hot path). `CanonicalKmerOf::partition()` added alongside it (wraps
`minimizer().seq_hash() & mask`, the same routing rule
`KmerPartition`/`RoutableSuperKmer` use).
- Cross-partition resolution (`outgoing.par_iter()` in
`build_layer_sibling_annex`) parallelised at the *partition* level — one
Rayon task per non-empty `outgoing[dest]` bucket. For k=31/m=11, a
central-base substitution changes the winning minimiser (and thus the
destination partition) only when that window overlaps the central base:
~11 of the 21 possible windows do, so ~10/21 (≈48%) of generated variants
route right back to the partition already being built. That self bucket
ends up far larger than any other, so the per-partition split pinned one
thread to it alone while the rest of the pool finished instantly —
confirmed by sampling: one thread solid in `MphfLayer::find`, everyone
else idle. Fixed by splitting each non-empty bucket into
`total_queries / n_workers` (capped 4096) chunks *before* `par_iter()`,
preserving per-partition mmap locality (each chunk stays contiguous
within one partition) while letting Rayon spread an oversized bucket
across several threads. Net effect of both fixes together: ~3 cores
average → ~10-13 cores average on the same run, and a projected total
build time of ~1h15 down to ~30min on the real `phyloskims_sal_vac` run
this was measured against.
- `TracedBar`'s ETA (`obisys/src/progress.rs`) was silently starved: the
custom progress message and the self-computed ETA text used to share one
`pb.set_message()` slot, with the ETA holding off for 2s after any custom
message — fine when custom messages are rare, broken once
`build_sibling_annex`'s per-partition callback fires more often than
that. Fixed by keeping the two texts in separate fields, composed
together on every render instead of one overwriting the other.
**Tried and reverted — do not repeat blindly:**
- Parallelising the *outer* partition loop in `build_sibling_annex` with
`obikindex::PartitionRunner` (already used by `merge`/`build_layers`),
splitting a fixed core budget between outer (partition) and inner
(pipeline + resolution) concurrency so their product wouldn't exceed the
budget. Measured *worse*: throughput dropped over time (26
partitions/5min → 38/11-12min) and peak resolution concurrency fell from
~11-12 cores to ~7-8. Cause: this capped the resolution burst — which
scales very well on its own — to make room for outer concurrency, and
running several partitions' resolution at once scatters access across
multiple partitions' mmap regions at once, working against the
locality `outgoing`'s per-partition grouping exists for. `PartitionRunner`
stayed exported from `obikindex` (`new_capped` too) since it's
general-purpose, but nothing in `obikphylo` calls it.
- Splitting resolution chunks even finer (`/(n_workers*8)`, cap 1024,
instead of `/n_workers`, cap 4096) to smooth the residual sawtooth.
Measured ~10% *slower*, wider dips, not narrower. Reverted to the
original chunk sizing.
**Known remaining limitation, not yet worth fixing:** within one layer, the
four stages (sequential `unitigs.bin` read → parallel generation →
parallel resolution → sequential annex write) never overlap — confirmed by
1s-interval sampling: generation alone occupies ~17 threads evenly, but the
next layer's read/generation never starts until the current layer's
resolution and write are both done. This produces a real, periodic (~layer
duration) alternation between "many cores" and "few cores" that neither of
the fixes above touches, since both operate *within* one layer's resolution
step. The only remaining lever is overlapping consecutive layers (e.g. a
depth-2 pipeline: start layer N+1's read/generation while layer N's
resolution/write is still running) — a real restructuring, not a parameter
tweak, and explicitly *not* to be combined with the reverted
budget-capping idea above (let each phase use however many cores it
naturally wants; only the *scheduling* needs to overlap). Deferred, not
started.