fix(read): decode on the calling thread when rayon's pool has one thread

Full reads of chunked datasets handed their chunks to rayon. With a
one-thread pool (concurrent_read --decode-threads 1, RAYON_NUM_THREADS=1)
every thread reading through a File queued behind that single worker, so
16 readers decoded on one core: per-thread CPU time showed one thread
doing all the decoding and the readers almost none, and full reads
stopped at about 2x one thread. The cached full-read path and the
uncached reader behind verify_provenance now decode inline when the pool
cannot parallelise (parallel_read::pool_can_parallelise).

The File's chunk cache was the suspect but not the cause: datasets over
its budget were already read without inserting, and skipping its lookups
gained only a few percent at 16 threads.

The regression test keeps a one-thread global pool's worker busy and
requires a full read and verify_provenance to finish anyway; before the
fix both waited for the worker (timed out).

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
This commit is contained in:
osobh
2026-09-26 08:40:10 -05:00
co-authored by Claude Opus 5.5
parent 63648c7000
commit a5e41c1a53
6 changed files with 152 additions and 14 deletions
+11 -7
View File
@@ -40,6 +40,7 @@ fn decompress_all_chunks(
{
if let Some(pl) = pipeline
&& parallel_read::should_use_parallel(chunks.len())
&& parallel_read::pool_can_parallelise()
{
// Seed from the first chunk's address and count for determinism.
let seed = chunks.first().map(|c| c.address).unwrap_or(0) ^ (chunks.len() as u64);
@@ -1056,7 +1057,9 @@ pub fn read_chunked_data_cached(
// Decompress what the cache didn't have, a bounded batch at a time — in
// parallel with the `parallel` feature (this path, the one the facade
// uses, was sequential; only the uncached reader was parallel). Chunks are
// uses, was sequential; only the uncached reader was parallel), unless the
// pool has one thread: then every reading thread would queue behind that
// one worker, so each decodes its own chunks instead. Chunks are
// cached only when the whole dataset fits: pushing a larger dataset
// through the cache just evicts each chunk moments after inserting it.
let cache_them = total_bytes <= cache.max_bytes();
@@ -1073,12 +1076,13 @@ pub fn read_chunked_data_cached(
};
for batch in misses.chunks(DECODE_BATCH) {
#[cfg(feature = "parallel")]
let decoded: Vec<Result<Vec<u8>, FormatError>> = if batch.len() >= 4 {
use rayon::prelude::*;
batch.par_iter().map(decode).collect()
} else {
batch.iter().map(decode).collect()
};
let decoded: Vec<Result<Vec<u8>, FormatError>> =
if batch.len() >= 4 && parallel_read::pool_can_parallelise() {
use rayon::prelude::*;
batch.par_iter().map(decode).collect()
} else {
batch.iter().map(decode).collect()
};
#[cfg(not(feature = "parallel"))]
let decoded: Vec<Result<Vec<u8>, FormatError>> = batch.iter().map(decode).collect();
@@ -27,6 +27,20 @@ pub fn should_use_parallel(chunk_count: usize) -> bool {
chunk_count > PARALLEL_THRESHOLD
}
/// Whether handing a read's chunks to rayon can decode them faster than the
/// calling thread would alone.
///
/// `false` when the pool the work would go to (the current pool inside a
/// rayon worker, else the global one) has a single thread. Handing work to
/// that pool is then worse than useless: the caller blocks while the one
/// worker decodes, and every other thread reading at the same time queues
/// behind the same worker, so N reader threads decode on one core. (That is
/// how full reads with `--decode-threads 1` stopped scaling at about 2x in
/// the `concurrent_read` benchmark.)
pub fn pool_can_parallelise() -> bool {
rayon::current_num_threads() > 1
}
/// Decompress chunks in parallel using lane-partitioned assignment.
///
/// Instead of naive `par_iter`, chunks are deterministically assigned to lanes
@@ -0,0 +1,90 @@
//! With a one-thread rayon pool, full reads of chunked datasets must decode
//! on the calling thread.
//!
//! Handing a read's chunks to a one-worker pool made every reading thread
//! queue behind that single worker: N threads reading through one `File`
//! decoded on one core, and full reads stopped scaling at about 2x in the
//! `concurrent_read` benchmark with `--decode-threads 1` (see
//! `docs/known-issues.md`). The test makes that queueing observable: it keeps
//! the pool's only worker busy and requires reads to finish anyway.
//!
//! One test in its own binary: it configures the process-wide rayon pool.
#![cfg(feature = "parallel")]
use std::sync::mpsc;
use std::time::Duration;
use clawhdf5::{File, FileBuilder};
const N: usize = 4096; // 64 chunks of 64 elements
fn values() -> Vec<f64> {
(0..N).map(|i| i as f64 * 0.5).collect()
}
fn build() -> File {
let mut b = FileBuilder::new();
b.create_dataset("data")
.with_f64_data(&values())
.with_shape(&[N as u64])
.with_chunks(&[64])
.with_deflate(1)
.with_provenance("test-suite", "2026-09-26T00:00:00Z", None);
File::from_bytes(b.finish().unwrap()).unwrap()
}
/// Run `f` on a fresh thread; `None` if it has not finished within `limit`.
fn finishes_within<T: Send + 'static>(
limit: Duration,
f: impl FnOnce() -> T + Send + 'static,
) -> Option<T> {
let (tx, rx) = mpsc::channel();
std::thread::spawn(move || {
let _ = tx.send(f());
});
rx.recv_timeout(limit).ok()
}
#[test]
fn full_reads_do_not_wait_for_a_busy_one_thread_pool() {
rayon::ThreadPoolBuilder::new()
.num_threads(1)
.build_global()
.expect("this test binary configures the global pool first");
// Built first: the writer compresses on the pool too.
let file = std::sync::Arc::new(build());
let file2 = std::sync::Arc::clone(&file);
// Occupy the pool's only worker until the reads are done.
let (started_tx, started_rx) = mpsc::channel();
let (release_tx, release_rx) = mpsc::channel::<()>();
rayon::spawn(move || {
started_tx.send(()).unwrap();
let _ = release_rx.recv();
});
started_rx.recv().unwrap();
let limit = Duration::from_secs(20);
// Cached full read (the path `read_*` uses), then the uncached reader
// behind `verify_provenance`.
let read = finishes_within(limit, move || {
file.dataset("data").unwrap().read_f64().unwrap()
});
let verified = finishes_within(limit, move || {
file2.dataset("data").unwrap().verify_provenance().unwrap()
});
// Free the worker before asserting, so a failure does not hang the
// blocked reader threads forever.
release_tx.send(()).unwrap();
assert_eq!(
read.expect("a full read waited for the busy one-thread rayon pool"),
values()
);
assert_eq!(
verified.expect("verify_provenance waited for the busy one-thread rayon pool"),
clawhdf5::provenance::VerifyResult::Ok
);
}