Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
GPU device-to-device integration remains unverified, and preparation performs an unnecessary compiled execution.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Improves Reactant/CUDA interoperability by reusing staging buffers and enabling device-to-device output copies.
Changes:
- Adds staging and device-pointer interop hooks.
- Reuses Reactant input/output staging buffers.
- Adds CUDA and Reactant tests, documentation, and changelog updates.
| File | Summary |
|---|---|
test/test-Reactant-HVP.jl |
Tests staged Reactant execution. |
test/test-CUDA-Extension.jl |
Tests CUDA hooks and pointer aliasing. |
src/ParallelMCMC.jl |
Defines staging and pointer-wrapper hooks. |
ext/ReactantExt.jl |
Implements reusable buffers and device-result handling. |
ext/CUDAExt.jl |
Adds pinned memory and CUDA pointer wrapping. |
docs/src/15-gpu.md |
Documents updated GPU behavior. |
CHANGELOG.md |
Records the optimization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+123
to
+126
| view = _device_view(template, out, platform) | ||
| if view !== nothing | ||
| res = similar(template, eltype(out), size(out)) | ||
| GC.@preserve out copyto!(res, view) |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (5.88%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
- Coverage 89.54% 88.25% -1.29%
==========================================
Files 8 8
Lines 1282 1303 +21
==========================================
+ Hits 1148 1150 +2
- Misses 134 153 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rsenne
marked this pull request as draft
September 20, 2026 21:49
rsenne
force-pushed
the
reactant-device-buffers
branch
from
September 20, 2026 22:10
7ea1ef5 to
617ba3e
Compare
ReactantExt copied every compiled AutoReactant call through the host: Array(x), a new XLA upload, Array(out), then a copy into a new device array. With CuArray inputs on a CUDA XLA client that is two PCIe transfers and three allocations per call. Add two hooks next to needs_host_staging: _host_staging_buffer (CUDAExt returns pinned memory) and _device_array_from_pointer (CUDAExt wraps a "cuda" pointer as a non-owning CuArray). ReactantExt builds one callable per compiled function that owns reusable host stages and copies results on the device when the XLA buffer can be wrapped, falling back to the host stage otherwise. Inputs are still uploaded fresh each call: Reactant has no device-pointer import, and its copyto! into a ConcreteRArray runs a compiled device copy on top of the upload, so persistent XLA inputs would be slower. Results are still fresh arrays every call and keep eltype promotions. Closes #69
rsenne
force-pushed
the
reactant-device-buffers
branch
from
September 20, 2026 22:19
617ba3e to
b061c62
Compare
rsenne
changed the title
Copy device results through a copy-and-synchronize hook instead of handing back a non-owning view. The view was read by an async copyto! on CUDA.jl's stream, so GC.@preserve held the XLA buffer only until the copy was enqueued, not until it ran. The extension now owns the copy and synchronizes before returning. Drop the output staging buffer and the extra compiled call that sized it. The platform is known at compile time from the default client, and the fallback is a plain host download. Collapse the two arity-specific callables into one ReactantCall, and drop a try/catch that could not trigger. Share one StagedArray test double between the CUDA and Reactant test files.
rsenne
force-pushed
the
reactant-device-buffers
branch
from
September 21, 2026 13:46
abe7cda to
16ed9f2
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the host round trip in
ReactantExt(#69, and wsmoses's comment on #66).Closes #69