perf: parallelize the local collection pipeline (#287) (#299)
## Summary The `tokio::join!` in `collect_parallel_first_iteration` fanned out over nine arms but polled all of them on one task. Every arm except full-process collection called synchronous reader methods with no yield point, so the join only provided ordering and result aggregation: the arms took turns on a single worker and the cycle cost the sum of all of them. `collect_sequential`, the steady-state path used for every cycle after the first, was serial by construction and had exactly the same problem, so both paths are fixed here. Both now dispatch every synchronous reader pass to the blocking pool up front and join only at the end. Nothing synchronous is left on the async task in either path. ## Design Work is grouped per reader collection, not per query. GPU device info, GPU processes, vGPU and MIG stay back to back inside one task because they all run against the same `Box<dyn GpuReader>` instances, and those carry internal sampling state (IOReport deltas on Apple Silicon, cached NVML handles on NVIDIA); issuing them concurrently against one reader would change the order in which that state is touched, which is exactly what acceptance criterion 5 forbids. CPU, memory, chassis, storage and full-process collection each get their own task, giving six groups that genuinely overlap. **How the `Arc<RwLock<...>>` borrows are handled.** `spawn_reader_pass` takes the guard with `Arc<RwLock<T>>::read_owned()` in async context and moves it into the closure. This is what makes the refactor possible at all: `read()` is a future and unusable inside a blocking closure, while `blocking_read()` from the blocking pool would risk deadlocking against a queued async writer (tokio's `RwLock` is fair, so a queued writer blocks subsequent readers). `OwnedRwLockReadGuard<T>` is `'static + Send` whenever `T: Send + Sync`, and `GpuReader`/`CpuReader`/`MemoryReader`/`ChassisReader` are all declared `Send + Sync`, so the `spawn_blocking` bounds are satisfied without cloning or re-owning a single reader. No guard is ever held across an `.await`. **Why holding the guard for the task's lifetime is safe.** I checked every writer of these locks: the only one is `initialize_readers`, which runs once and completes before the first collection. There is no hotplug or periodic-refresh path that takes a write lock while a collection is in flight, so a long-lived read guard cannot starve anything. **The one dependency that is kept.** In the steady-state path `update_process_cache` needs this cycle's GPU pid set to decide which cached entries are GPU-attributed, so the process task is spawned after the GPU group joins rather than alongside it. Breaking that dependency would change collected values. Every other group is already in flight by then, so it only extends the critical path when GPU plus process collection outlasts all of them (it does not on the measured machine, and the code comment says so). The first-iteration path has no such dependency: it deliberately seeds the cache with an empty pid set, so all six groups start together there. Blocking-pool sizing was checked: six concurrent tasks per cycle sits far below the default `max_blocking_threads`, and below the `max_blocking_threads(32)` used by the snapshot runtime. ## Measurements Recorded on this machine (Apple M5 Max, 18 cores), release profile, via the `measure_collection_arms` harness added in `src/view/data_collection/local_collector/tests.rs`. The baseline was measured first, before any production code changed, by checking out `origin/main` into a separate worktree, appending the same harness to it, and building it with its own target directory. Two runs of each, alternating, on an otherwise idle machine. Wall clock and CPU are 100 back-to-back steady-state cycles divided by 100; CPU time is `getrusage(RUSAGE_SELF)` user + system, which counts every thread, so blocking-pool work is charged exactly like inline work. | | baseline run 1 | baseline run 2 | after run 1 | after run 2 | |---|---|---|---|---| | wall clock per cycle | 20.118 ms | 21.717 ms | 14.883 ms | 14.783 ms | | process CPU per cycle | 19.320 ms | 21.129 ms | 19.372 ms | 19.112 ms | Wall clock drops roughly 28%. CPU time is flat within run-to-run noise and never above baseline, which is the "no regression" check: `spawn_blocking` relocated the cycles rather than adding any. There is no busy-wait and no duplicated work; the same reader calls run the same number of times. Per-arm synchronous cost on this machine (mean of 12 samples, measured before the change): storage 14.371 ms, full process refresh 8.147 ms, GPU info 0.010 ms, CPU info 0.006 ms, memory 0.003 ms, chassis 0.002 ms, vGPU/MIG/GPU-processes below 0.001 ms. Serialized sum 22.541 ms. After the change the critical path is bounded by the storage pass, which matches the observed 14.8 ms. **Limitations, stated honestly.** This is a single-machine, single-platform measurement. On Apple Silicon after PR #286 the GPU reader is effectively free (10 microseconds), so the win here comes from overlapping storage with process collection rather than from getting the GPU arm off the worker. On an NVIDIA host where `get_gpu_info` costs tens of milliseconds the GPU group would dominate and the ratio would differ; I have no NVIDIA hardware here to measure that, and the numbers above should not be read as a cross-platform claim. The `measure_collection_arms` harness is committed (ignored by default) precisely so the numbers can be reproduced on other hardware. The first-iteration path is not in the table because its one-shot nature makes a 100-cycle average meaningless; it is covered by a functional test instead. ## What changed - `src/view/data_collection/local_collector.rs`: added `GpuCollection` plus `collect_from_gpu_readers` for the grouped GPU pass, added the `spawn_reader_pass` helper, rewrote `collect_parallel_first_iteration` to spawn six blocking groups instead of a nine-arm `tokio::join!`, and rewrote `collect_sequential` the same way. - Renamed `collect_sequential` to `collect_steady_state`, since the old name no longer describes what it does. - The first-iteration status updates now use `blocking_send` from inside the blocking closures, so each "collected" line still appears as its group finishes rather than all at once at the end. - Hardened the startup status handler to bound the shifted index (`3 + index`) instead of the raw one, so an out-of-range write cannot panic that task. - `src/view/data_collection/local_collector/tests.rs` (new): test module split out following the existing `#[path = ".../tests.rs"]` convention used by `cpu_linux`, `memory_linux`, `container_info` and others. ## Test plan - [x] `cargo fmt --check` - [x] `cargo check --lib --tests` - [x] `cargo clippy --lib --tests -- -D warnings` (clean) - [x] `cargo test --bin all-smi view::data_collection::local_collector::tests -- --test-threads=1` (3 passed, 1 ignored harness) - [x] `parallel_collection_matches_serial_reference`: runs the new pipeline and a straight-line reference collection back to back and asserts the GPU uuid set, CPU/memory/vGPU/MIG/chassis row counts, and storage mount-point set match, plus that the process list stays sorted by CPU descending and truncated to `MAX_DISPLAY_PROCESSES`. Absolute metric values are sampled at different instants and cannot be compared for equality, so everything that is not time varying is asserted instead. - [x] `repeated_collections_complete_without_deadlock`: runs `FULL_REFRESH_INTERVAL + 2` cycles under a timeout, covering both the selective and full process-refresh branches, to catch a lock or blocking-pool deadlock. - [x] `first_iteration_collection_reports_startup_status`: drives the real first-iteration path through `initialize_readers` and asserts all five arms report completion, which is also what proves `blocking_send` inside `spawn_blocking` does not panic. Closes #287
J
Jeongkyu Shin committed
cf05c0d78729a0eeb437f775a35c1d67e85a344e
Parent: 631f42a
Committed by GitHub <noreply@github.com>
on 7/31/2026, 7:41:53 AM