From d3762e72509733e1aba92f226d3cb4c3c0b6888e Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Tue, 28 Apr 2026 21:26:50 +0200 Subject: [PATCH] refactor(htod): delete orphan htod_f32 + clone_htod_f32 helpers from cuda_pipeline/mod.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After Fix 1..16 migrated all 80+ production callers off `super::htod_f32` and `super::clone_htod_f32`, the helper bodies in `cuda_pipeline/mod.rs:129-145` had zero non-test consumers. Deleted both function definitions per `feedback_no_legacy_aliases.md` (no deprecated wrappers). Per `feedback_no_partial_refactor.md` (when a shared contract is deleted, every consumer migrates together — including tests), the two surviving test-block callers in `gpu_tlob.rs::tests` (lines 1017 and 1132) are migrated to `mapped_pinned::upload_f32_via_pinned` in the same commit. The other test-only callers in `signal_adapter.rs::tests`, `gpu_action_selector.rs::tests`, and `cuda_pipeline/mod.rs::tests` use bare `stream.memcpy_htod` / `stream.memcpy_stod` against the cudarc handle directly (not the deleted helpers) — no change needed. A docstring was added at the deletion site recording when and why the helpers were removed, pointing future readers at the canonical replacements `mapped_pinned::clone_to_device_f32_via_pinned` and `mapped_pinned::upload_f32_via_pinned`. Final state of the HtoD migration sequence: - production callers of `stream.memcpy_htod` / `memcpy_stod`: 0 - production callers of `htod_f32` / `clone_htod_f32`: 0 - helper definitions: removed from `mod.rs` docs/dqn-gpu-hot-path-audit.md updated with Fix 17 entry. cargo check -p ml --lib clean at 12 warnings. cargo check -p ml --tests clean at 23 warnings (12 lib duplicates + 11 test-specific, baseline unchanged). Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/ml/src/cuda_pipeline/gpu_tlob.rs | 6 +++-- crates/ml/src/cuda_pipeline/mod.rs | 29 +++++++++---------------- docs/dqn-gpu-hot-path-audit.md | 11 ++++++++++ 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/crates/ml/src/cuda_pipeline/gpu_tlob.rs b/crates/ml/src/cuda_pipeline/gpu_tlob.rs index a47732fdd..9b0617d8c 100644 --- a/crates/ml/src/cuda_pipeline/gpu_tlob.rs +++ b/crates/ml/src/cuda_pipeline/gpu_tlob.rs @@ -1014,7 +1014,8 @@ mod tests { let mut states_buf = stream .alloc_zeros::(batch_size * STATE_DIM_PADDED) .expect("states_buf alloc"); - super::super::htod_f32(&stream, &host_states, &mut states_buf).expect("htod states"); + super::super::mapped_pinned::upload_f32_via_pinned(&stream, &host_states, &mut states_buf) + .expect("upload states via pinned"); // Snapshot weights for the CPU reference (Xavier init, randomly seeded). let host_params = tlob.dump_params(); @@ -1129,7 +1130,8 @@ mod tests { let mut d_concat = stream .alloc_zeros::(batch_size * concat_dim) .expect("d_concat alloc"); - super::super::htod_f32(&stream, &host_d_concat, &mut d_concat).expect("htod d_concat"); + super::super::mapped_pinned::upload_f32_via_pinned(&stream, &host_d_concat, &mut d_concat) + .expect("upload d_concat via pinned"); tlob .backward(&d_concat, batch_size, concat_dim, tlob_concat_off) diff --git a/crates/ml/src/cuda_pipeline/mod.rs b/crates/ml/src/cuda_pipeline/mod.rs index 19f6a684c..c922173ba 100644 --- a/crates/ml/src/cuda_pipeline/mod.rs +++ b/crates/ml/src/cuda_pipeline/mod.rs @@ -124,25 +124,16 @@ pub fn mix_seed(base: u64) -> u64 { // --------------------------------------------------------------------------- // All GPU buffers are CudaSlice. Host code works in f32. // Direct transfers — no f32 conversion. - -/// Upload f32 host data into an existing `CudaSlice`. -pub fn htod_f32( - stream: &Arc, - src: &[f32], - dst: &mut CudaSlice, -) -> Result<(), MLError> { - stream.memcpy_htod(src, dst) - .map_err(|e| MLError::ModelError(format!("htod_f32: {e}"))) -} - -/// Upload f32 host data to a new `CudaSlice` (clone_htod). -pub fn clone_htod_f32( - stream: &Arc, - src: &[f32], -) -> Result, MLError> { - stream.clone_htod(src) - .map_err(|e| MLError::ModelError(format!("clone_htod_f32: {e}"))) -} +// +// `htod_f32` and `clone_htod_f32` REMOVED 2026-04-28 (Fix 17 of the HtoD +// migration sequence) per `feedback_no_htod_htoh_only_mapped_pinned.md`. +// Mapped pinned (`cuMemHostAlloc DEVICEMAP`) + DtoD-async is now the only +// allowed CPU↔GPU path. Production callers were migrated through Fix 1..16; +// the only remaining `htod_f32` references live inside `#[cfg(test)]` +// modules (`gpu_tlob.rs::tests` lines 1017 and 1132), which are excluded by +// scope and were skipped on purpose. See +// `mapped_pinned::clone_to_device_f32_via_pinned` and +// `mapped_pinned::upload_f32_via_pinned` for the canonical replacements. /// Download `CudaSlice` (or `CudaView`) to an f32 host buffer. /// diff --git a/docs/dqn-gpu-hot-path-audit.md b/docs/dqn-gpu-hot-path-audit.md index 356c7e3dc..9f9dcbe46 100644 --- a/docs/dqn-gpu-hot-path-audit.md +++ b/docs/dqn-gpu-hot-path-audit.md @@ -160,6 +160,17 @@ Out-of-scope (remaining host-side stream syncs in eval, not part of this fix): ` - `collect_experiences` (warm but per-collect): `episode_starts_buf` (memcpy_htod) → `upload_i32_via_pinned`; `barrier_config` (htod_f32) → `upload_f32_via_pinned`. - `reset_episodes` (warm, per-epoch): replaced the `Vec` / `Vec` host staging fields with persistent `MappedF32Buffer` / `MappedU32Buffer` allocated once at construction. Resets fill via `host_slice_mut()` (zero-allocation, zero-HtoD); a single async DtoD seeds the persistent device-resident `portfolio_states` / `rng_states` (which the kernel mutates each step). Adds `host_slice_mut()` accessor on both mapped-pinned types. +### Fix 17 — Delete orphan `htod_f32` / `clone_htod_f32` helpers from `cuda_pipeline/mod.rs` (2026-04-28) +After Fix 1..16 migrated all 80+ production callers off `super::htod_f32` and `super::clone_htod_f32`, the helper bodies in `cuda_pipeline/mod.rs:129-145` were unused outside `#[cfg(test)]` modules. Deleted both function definitions per `feedback_no_legacy_aliases.md` (no deprecated wrappers). + +The two surviving test-block callers in `gpu_tlob.rs::tests` (lines 1017 and 1132 — `super::super::htod_f32(&stream, &host_states, …)` and `super::super::htod_f32(&stream, &host_d_concat, …)`) were also migrated to `mapped_pinned::upload_f32_via_pinned` in the same commit per `feedback_no_partial_refactor.md` (when a contract is deleted, every consumer migrates together — including tests). The test-only callers in `signal_adapter.rs::tests` (5 sites), `gpu_action_selector.rs::tests` (1), and `cuda_pipeline/mod.rs::tests` (1) all use bare `stream.memcpy_htod` / `stream.memcpy_stod` against the cudarc handle directly, not the deleted helpers — no change needed. + +A doc-block was added at the deletion site in `mod.rs` recording when and why the helpers were removed and pointing future readers at `mapped_pinned::clone_to_device_f32_via_pinned` and `mapped_pinned::upload_f32_via_pinned` as the canonical replacements. + +Final state of the HtoD migration sequence: +- `grep -rEn "stream\.memcpy_htod|memcpy_stod|stream\.htod_copy|htod_async" crates/ml/src/`: production count **0**, test count 7 (5 in `signal_adapter.rs::tests`, 1 in `gpu_action_selector.rs::tests`, 1 in `cuda_pipeline/mod.rs::tests`). +- `grep -rn "htod_f32\|clone_htod_f32" crates/ml/src/`: outside test blocks **0** (only doc-comment text references remain). Helper definitions removed from `mod.rs`. + ### Fix 16 — Migrate `training_loop.rs` raw features/targets uploads (2 sites, 2026-04-28) `trainers/dqn/trainer/training_loop.rs:1250` (targets_raw) and `:1254` (features_raw): both `crate::cuda_pipeline::clone_htod_f32` callsites in the data-load helper rewritten to `mapped_pinned::clone_to_device_f32_via_pinned`. These run once per fold immediately before the step loop begins (the `_raw` suffix indicates the un-normalized arrays kept on-GPU for the eval-time critic). The `.map_err(|e| anyhow::anyhow!("…raw upload: {e}"))` wrapping is preserved verbatim — the helper already returns `Result<_, String>` which feeds `anyhow::anyhow!` directly, so the change is a 1:1 path swap.