add some doc about optimisation for query

This commit is contained in:
Eric Coissac
2026-08-20 13:59:12 +02:00
parent 89ea077456
commit a4eb20e67e
60 changed files with 3521 additions and 13 deletions
@@ -944,6 +944,34 @@
<li class="md-nav__item">
<a href="../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
@@ -948,6 +948,34 @@
<li class="md-nav__item">
<a href="../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
@@ -946,6 +946,34 @@
<li class="md-nav__item">
<a href="../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
@@ -948,6 +948,34 @@
<li class="md-nav__item">
<a href="../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
+28
View File
@@ -944,6 +944,34 @@
<li class="md-nav__item">
<a href="../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
+28
View File
@@ -944,6 +944,34 @@
<li class="md-nav__item">
<a href="../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
@@ -944,6 +944,34 @@
<li class="md-nav__item">
<a href="../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
@@ -944,6 +944,34 @@
<li class="md-nav__item">
<a href="../../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
@@ -9,7 +9,7 @@
<link rel="prev" href="../../../implementation/obitaxonomy/">
<link rel="prev" href="../../../implementation/benchmark_query_testing/">
<link rel="next" href="../../index_architecture/">
@@ -948,6 +948,34 @@
<li class="md-nav__item">
<a href="../../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
+127
View File
@@ -948,6 +948,34 @@
<li class="md-nav__item">
<a href="../../implementation/benchmark_query_testing/" class="md-nav__link">
<span class="md-ellipsis">
Benchmark: query-path testing
</span>
</a>
</li>
</ul>
</nav>
@@ -1289,6 +1317,17 @@
</span>
</a>
</li>
<li class="md-nav__item">
<a href="#query-never-benefits-from-sparse-row-major-access-found-2026-08-19-not-implemented" class="md-nav__link">
<span class="md-ellipsis">
query never benefits from sparse row-major access (found 2026-08-19, not implemented)
</span>
</a>
</li>
</ul>
@@ -1552,6 +1591,17 @@
</span>
</a>
</li>
<li class="md-nav__item">
<a href="#query-never-benefits-from-sparse-row-major-access-found-2026-08-19-not-implemented" class="md-nav__link">
<span class="md-ellipsis">
query never benefits from sparse row-major access (found 2026-08-19, not implemented)
</span>
</a>
</li>
</ul>
@@ -2319,6 +2369,83 @@ scan the full unsampled index — never threaded <code>--subsample</code>/<code>
out of scope here since the reported problem was specifically about the
<code>--sankoff</code>/<code>--tnt</code> pipeline's redundant/inconsistent scans, not these
two standalone flags.</p>
<h2 id="query-never-benefits-from-sparse-row-major-access-found-2026-08-19-not-implemented"><code>query</code> never benefits from sparse row-major access (found 2026-08-19, not implemented)</h2>
<p>Benchmarked <code>obikmer query</code> against <code>global_index_presence</code> (dense-packed)
vs. <code>global_index_presence_sparse</code> (<code>pack --sparse</code>), 100k simulated reads
× 2 specimens (<code>benchmark/</code>, see
<a href="../../implementation/benchmark_query_testing/">benchmark_query_testing.md</a>).
Correctness: 0 mismatches — sparse and dense return bit-identical query
results. Performance: sparse consistently <em>slower</em> than dense (~30-50%,
reproducible across two runs with warm disk cache), the opposite of
<code>pack --sparse</code>'s stated intent ("faster for single-row access... like
query").</p>
<p><strong>Root cause, read from source, not measured in isolation:</strong>
<code>KmerPartition::query_partition_with</code> (<code>obikpartitionner/src/query_layer.rs:155-220</code>)
is architecturally column-major: stage 2 walks <code>for g in 0..n_cols { for
slot in hit_slots { layer.col_value(g, slot) } }</code>, documented (correctly)
as the right locality strategy for the packed/columnar formats, where
<code>col_value</code><code>PersistentBitMatrix::get</code> is a genuine O(1) mmap'd column
read (<code>persistent.rs:110-113</code>).</p>
<p>For <code>Self::Sparse</code>, that same <code>get(c, slot)</code> (<code>persistent.rs:114-118</code>)
allocates a full <code>n_cols</code>-wide buffer and calls <code>fill_row</code> — materializing
the <em>entire row</em> — just to return one cell. Called from inside the
column-major double loop, this reconstructs the same row once per genome
column touched: O(hits × n_cols) full-row rebuilds instead of O(hits).
<code>PersistentSparseBitMatrix</code>'s own native row-major decode
(<code>for_each_genome_in_row</code>, <code>sparse.rs:164-177</code>, used correctly by its own
<code>row</code>/<code>fill_row</code>/<code>fill_sub_matrix</code>) is never reached from the query path
at all.</p>
<p><strong><code>fill_sub_matrix</code> (the existing <code>BinaryMatrix</code> trait primitive,
<code>traits.rs:13-37</code>) is not the right replacement for <code>query</code> either</strong>, even
once its own dispatch bug is fixed (see next section) — its output shape
is inherently column-dense: <code>out[col]</code> gets an entry for every column,
including columns with zero hits among the requested slots. On real
sparse data (a hit typically touching a handful of genomes out of dozens)
that's still O(n_cols) output regardless of true sparsity. What <code>query</code>
actually wants is the sparse triple stream <code>(slot, col, value)</code> it already
consumes as <code>QueryHit::Value</code> — not a materialized sub-matrix.</p>
<p><strong>Proposed primitive</strong> (design only, not implemented — explicit ask: keep
count matrices <em>not excluded</em>, even though effort right now is
presence/absence only):</p>
<div class="highlight"><pre><span></span><code><span class="sd">/// Visit every nonzero cell among `slots`. Order unspecified.</span>
<span class="k">fn</span><span class="w"> </span><span class="nf">for_each_nonzero</span><span class="p">(</span><span class="o">&amp;</span><span class="bp">self</span><span class="p">,</span><span class="w"> </span><span class="n">slots</span><span class="p">:</span><span class="w"> </span><span class="kp">&amp;</span><span class="p">[</span><span class="kt">usize</span><span class="p">],</span><span class="w"> </span><span class="n">f</span><span class="p">:</span><span class="w"> </span><span class="nc">impl</span><span class="w"> </span><span class="nb">FnMut</span><span class="p">(</span><span class="kt">usize</span><span class="w"> </span><span class="cm">/*idx into slots*/</span><span class="p">,</span><span class="w"> </span><span class="kt">usize</span><span class="w"> </span><span class="cm">/*col*/</span><span class="p">,</span><span class="w"> </span><span class="kt">u32</span><span class="w"> </span><span class="cm">/*value*/</span><span class="p">));</span>
</code></pre></div>
<ul>
<li>On <code>PersistentSparseBitMatrix</code>: native override, one pass per slot via
the existing (currently private) <code>for_each_genome_in_row</code> — O(Σ row
nnz), zero n_cols-wide allocation. This is the whole point: expose code
that already exists rather than write anything new for the sparse side.</li>
<li>On <code>PersistentBitMatrix::{Packed,Columnar,Implicit}</code>: <strong>provided
default</strong>, derived from <code>fill_sub_matrix</code> (materialize, then filter to
true cells) — reuses the already-optimal column-major/mmap path for
those formats, no new code needed there either.</li>
<li><strong>On <code>PersistentCompactIntMatrix</code> (counts)</strong>: same provided-default
treatment, derived from its own existing <code>fill_sub_matrix</code> (<code>u32</code>-typed
already, <code>intmatrix.rs:387</code>) — not hand-optimized (no sparse count
format exists — "Explicitly deferred" per <code>traits.rs:9-12</code>), but not
excluded either: it gets a working, not-pathological implementation for
free today, on the same trait, ready for a native override the day a
sparse count format lands. This is why the signature carries <code>u32</code>
rather than <code>bool</code> — presence is <code>0</code>/<code>1</code>, counts are <code>u32</code>, one trait
covers both without a bool/u32 split forcing counts out of the design.</li>
</ul>
<p>Would let <code>query_partition_with</code>'s stage 2 collapse to one
<code>layer.matrix().for_each_nonzero(&amp;hit_slots, |i, g, v| on_event(...))</code>
call per layer, format-agnostic, with each backend's existing (or
default-derived) implementation deciding the actual access pattern.</p>
<p><strong>Separately, an existing bug in the generic path</strong> (found while tracing
this, itself not yet fixed): <code>PersistentBitMatrix::fill_sub_matrix</code>
(<code>persistent.rs:190-215</code>, the enum wrapper backing <code>BinaryMatrix</code>'s
default trait impl) does <em>not</em> delegate to
<code>PersistentSparseBitMatrix::fill_sub_matrix</code> for <code>Self::Sparse</code> — it
reimplements the same naive per-(column, slot) <code>fill_row_bool</code> loop
instead, bypassing the efficient native method one file over
(<code>sparse.rs:249-258</code>). <code>obikphylo::siblings::cache::Mat</code>
(<code>cache.rs:138-145</code>) independently built its own parallel enum wrapper
that dispatches correctly — a sign this was worked around rather than
fixed at the source. Any future <code>for_each_nonzero</code> work should fix this
dispatch too (or route through it), rather than adding a third
independently-dispatching wrapper.</p>