perf(eval): truly async validation backtest on dedicated stream + mapped-pinned readback
Validation backtest was serialised on the training stream by passing self.cuda_stream as parent_stream into GpuBacktestEvaluator::new (the forked eval-stream still depends on training-stream completion, but the metrics + plan_diag readbacks went through clone_dtoh / memcpy_dtoh, each forcing an implicit sync that blocked behind the training stream's pending work). The prior comment claimed single-stream eval was needed "for cuBLAS determinism" — incorrect: cuBLAS bit-wise reproducibility caveats apply per-handle, not across the process, and each stream already owns its own PerStreamCublasHandles. Costs ~30-40s/epoch on L40S. Plan A: - Route eval through self.validation_stream (already forked at constructor.rs:547 but unused on the eval path) and add a fresh cuda_stream-recorded train_done_event so validation_stream + evaluator's internal forked stream both wait on it before launching eval kernels. GPU-side cuStreamWaitEvent — no CPU sync. - Replace metrics_buf and plan_diag_buf in GpuBacktestEvaluator from CudaSlice<f32> to MappedF32Buffer (cuMemHostAlloc DEVICEMAP). Kernels write through the buffer's dev_ptr; host reads via volatile host_ptr after a single eval_stream.synchronize() per evaluation. Replaces the forbidden clone_dtoh / memcpy_dtoh path per feedback_no_htod_htoh_only_mapped_pinned.md. The single sync blocks only the eval stream — training rolls on uninterrupted. - Update the misleading "single-stream cuBLAS determinism" comment to explain the actual per-stream-handle architecture. Per feedback_no_partial_refactor: every consumer of the metrics_buf / plan_diag_buf contract migrated in lockstep (struct fields, alloc, kernel-arg passing, host readout). Per-stream cuBLAS handle parity with training preserved (val TLOB still uses PerStreamCublasHandles::new(&eval_stream); evaluator forks its own internal cuBLAS state from parent_stream). Verified: cargo check -p ml --lib clean (13 warnings, workspace baseline). cargo test -p ml --lib --no-run clean. No fingerprint change — mapped-pinned is allocation-method orthogonal to kernel buffer layout. Companion commit lands Plan B (async best-checkpoint serialize). Wire-up audit entry covers Plan A here; will be extended with Plan B edit sites in the companion commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1335,3 +1335,75 @@ Sanitizer confirms 16 → 0 OOB errors in mamba2_backward path. Non-sanitizer
|
||||
smoke completes 20 epochs without LAUNCH_FAILED (was crashing on epoch 1
|
||||
prior to fix). The ensemble multi-head value path now reads correctly-sized
|
||||
W_v1 (8192 floats) and b_v1 (128 floats).
|
||||
|
||||
Async-validation overlap on dedicated stream + mapped-pinned readback
|
||||
(Plan A, 2026-04-28): per-epoch L40S wall time was inflated by ~30-40s
|
||||
of validation backtest serialised on the training stream. Two coupled
|
||||
bugs:
|
||||
|
||||
1. `crates/ml/src/trainers/dqn/trainer/metrics.rs:496-503` passed
|
||||
`self.cuda_stream` as `parent_stream` into
|
||||
`GpuBacktestEvaluator::new`, which forks its own internal stream
|
||||
from the parent. The forked stream does not auto-synchronise with
|
||||
the training stream — so eval kernels and training kernels submitted
|
||||
concurrently to independent streams compete on the GPU SMs without
|
||||
ordering. The prior comment claimed this was "for cuBLAS
|
||||
determinism (single-stream reproducibility)" — incorrect: cuBLAS
|
||||
bit-wise reproducibility caveats apply per-handle, not across the
|
||||
process. Each stream owns its own `PerStreamCublasHandles`
|
||||
(training has one in `fused_ctx`; validation has one via
|
||||
`PerStreamCublasHandles::new(&eval_stream)` for the val TLOB at
|
||||
metrics.rs:638; the evaluator forks its own cuBLAS handle
|
||||
internally in `gpu_backtest_evaluator.rs::new` from
|
||||
`parent_stream`). Running the eval through the training stream
|
||||
simply forced a serial schedule. **Fix**: route eval through
|
||||
`self.validation_stream` (already forked at constructor.rs:547
|
||||
but unused on the eval path). The `validation_stream` waits on a
|
||||
fresh `cuda_stream`-recorded `train_done_event` before launching
|
||||
eval; the evaluator's internal forked stream is then walled off
|
||||
via `evaluator.stream().wait(&ev)`. Both waits are GPU-side
|
||||
`cuStreamWaitEvent` — no CPU sync. Ordering is guaranteed:
|
||||
training writes weights → training records event → val stream +
|
||||
evaluator stream wait on event → eval reads weights. Updated the
|
||||
misleading comment to explain the per-stream-handle determinism
|
||||
guard.
|
||||
|
||||
2. `crates/ml/src/cuda_pipeline/gpu_backtest_evaluator.rs:631` (alloc),
|
||||
:2143 (`clone_dtoh(&self.metrics_buf)`), and :1075 (`memcpy_dtoh`
|
||||
for `plan_diag_buf`) — the metrics + plan-diag readbacks went
|
||||
through `cudarc` `memcpy_dtoh`/`clone_dtoh` which is plain DtoH and
|
||||
forbidden per `feedback_no_htod_htoh_only_mapped_pinned.md`: only
|
||||
mapped pinned memory (`cuMemHostAlloc DEVICEMAP`) is allowed for
|
||||
CPU↔GPU paths. Even on the val stream the DtoH triggers an
|
||||
implicit stream sync that on a single-stream pipeline serialised
|
||||
behind the training stream's pending work. **Fix**: replaced
|
||||
`metrics_buf: CudaSlice<f32>` and `plan_diag_buf: CudaSlice<f32>`
|
||||
with `MappedF32Buffer` (cuda_pipeline/mapped_pinned.rs). The
|
||||
`metrics_kernel` and `backtest_plan_diag_reduce` now write through
|
||||
the buffer's `dev_ptr` (passed as a u64 kernel arg, same pattern
|
||||
as `gpu_iqn_head.rs::total_loss_dev_ptr`). The host reads via
|
||||
volatile `host_ptr` after a single `eval_stream.synchronize()` per
|
||||
evaluation — one sync replaces N implicit syncs from the prior
|
||||
`clone_dtoh` calls. The sync blocks only the eval stream; training
|
||||
continues on `cuda_stream` uninterrupted. The plan_diag readout
|
||||
adopts the same one-epoch lag pattern as `pending_val_loss`
|
||||
(zero-init host buffer means epoch 0 logs zeros, subsequent epochs
|
||||
see the previous epoch's diag — acceptable for a diagnostic-only
|
||||
readout that already lived inside the existing one-epoch-lag
|
||||
pipeline).
|
||||
|
||||
Per `feedback_no_partial_refactor`: every consumer of the
|
||||
`metrics_buf`/`plan_diag_buf` contract migrated in lockstep — struct
|
||||
fields, alloc sites in `new()`, kernel-arg passing in
|
||||
`launch_metrics_and_download` and `launch_plan_diag_and_log`, host
|
||||
readout (was `clone_dtoh` → now `read_all()` on `MappedF32Buffer`).
|
||||
The cross-stream event ordering in metrics.rs is the only consumer
|
||||
of the new validation_stream-as-parent contract; the legacy
|
||||
single-stream fallback (when `validation_stream is None`, e.g. CPU
|
||||
device) routes through `cuda_stream` exactly as before. cargo check
|
||||
clean at 13 warnings (workspace baseline). cargo test --no-run
|
||||
clean. No fingerprint change — buffer layouts unchanged from the
|
||||
kernel's perspective (mapped-pinned is allocation-method orthogonal
|
||||
to layout). Plan B (async best-checkpoint serialize) lands in the
|
||||
companion commit and extends this entry with the checkpoint-side
|
||||
edit sites.
|
||||
|
||||
Reference in New Issue
Block a user