Repository navigation
Reuse device streams across threads when graph capture is disabled - #33053
Tianlei Wu (tianleiwu) with Copilot wants to merge 13 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: tianleiwu <30328909+tianleiwu@users.noreply.github.com>
Co-authored-by: tianleiwu <30328909+tianleiwu@users.noreply.github.com>
Co-authored-by: tianleiwu <30328909+tianleiwu@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Cross-thread MIGraphX stream reuse can allocate input buffers on the wrong GPU.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Reduces per-thread GPU memory retention by sharing idle device streams when graph capture is disabled, addressing #33037.
Changes:
- Preserves thread-affine pooling for graph capture.
- Selects stream devices before creating notifications.
- Adds tests for reuse, thread affinity, and overlapping acquisitions.
| File | Description |
|---|---|
onnxruntime/test/framework/session_state_test.cc |
Adds stream-pooling and device-selection tests. |
onnxruntime/core/framework/stream_execution_context.cc |
Selects devices before notification creation. |
onnxruntime/core/framework/session_state.h |
Adds pooling-policy state and comments. |
onnxruntime/core/framework/session_state.cc |
Implements conditional sharing and pruning. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: tianleiwu <30328909+tianleiwu@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Select the owning device when borrowing cached streams, before input copies can allocate. Cover cross-thread and same-thread device restoration, plus real CUDA async copies, notification destruction, and deferred pinned-buffer cleanup across caller exit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Re-reviewed the shared-pool gating, exclusive collection borrowing, device restoration before allocation/notification creation, and deferred-buffer lifetime tests. The prior MIGraphX/thread-affinity concern remains addressed, and I found no additional actionable issues in the current diff.
The PR records focused validation of the earlier implementation, but there are no reported checks for this merge head. I did not treat an existing build of another revision as current-head validation. Please obtain the current-head checks before merging. No approval submitted.
Preserve the cross-thread stream-pool coverage and main's iterator-based framework tests while incorporating the validated CUDA self-copy and dependency CI repairs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Final maintenance verification at The stream-pool main-merge repair and shared React Native lifetime/navigation fixes remain intact. The original AsyncWorker header's real pinned-Hermes/JSI ASan UAF was reproduced when resolve dispatch released the final worker before its post-dispatch completion hook. The retained The Windows CPUINFO repair changes only two regression-test files. It checks allocation cleanup after CPUIDInfo destruction and before later CRT/loader address reuse, repeats16 unload cycles and preserves process-lifetime result storage when a retained module delays callbacks. Five actual-source native-loader protocol fixtures passed, but the decisive proof is the actual current86 Windows debug42610467134: real job129137252263 reports the mandatory shared-lib CPUINFO detector Passed in3.97seconds and35/35 CTest cases passed. Protocol mocks and older-head outcomes are not substituted for that Windows runtime execution. The actual current86 React Native42610467299 completed SUCCESS. Fresh Android artifact13671586695 and iOS artifact13672671135 each verify exactly three real passing MNIST, Basic Types and EPContext cases, no failures/errors/skips and the unchanged14/14 checks passed assertion in device logs/traces. Historical172 artifacts and superseded infrastructure retries are not used as current86 proof. No tests, tolerances, teardown checks or runtime assertions were bypassed. |
Target the accessible pressable instead of the wrong child text view selected on iOS. Exercise the actual Detox scenario on both platforms and retain the aggregate success assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Run the native completion hook before dispatch can release the final worker reference. Preserve exactly-once completion when dispatch throws, and assert completion at promise settlement in the EPContext bridge regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The iOS Button accessibility label is duplicated on native wrappers. Keep its accessibility label but identify the pressable with a unique testID. Reproduce the duplicate-label failure, verify the actual rendered native identifier and navigation, and preserve the complete 14/14 device assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Observe the allocation after CPUIDInfo destruction and before later CRT/loader cleanup can reuse its address. Keep strict deallocation validation, repeat 16 load/unload cycles, and retain observer result storage safely when another module reference prevents unloading. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Description
Local validation
python3 build/pr33053-cuda/build_focused_session_state_tests.py— passed. The isolated focused runner uses the actualsession_state_test.ccand original test main, linked with the original CMake production-library recipe.cd build/pr33053-cuda/Release && LD_LIBRARY_PATH="$PWD:/usr/local/cuda/lib64" timeout 120s ./onnxruntime_session_state_focused_test --gtest_filter='SessionStateTest.DeviceStreamPool*' --gtest_repeat=3— passed all four tests in each of three iterations (12 passes, no skips), including real CUDA asynchronous copies on GPU 1.build/pr33053-cuda/tools-venv/bin/clang-format --dry-run --Werror onnxruntime/core/framework/session_state.cc onnxruntime/core/providers/cuda/cuda_stream_handle.cc onnxruntime/test/framework/session_state_test.cc— passed with repository-pinned clang-format 20.1.8.git diff --check— passed.Validation used an isolated CUDA-minimal build with two build jobs and one nvcc worker. An existing unused-parameter warning in the CUDA-minimal per-thread-context constructor was downgraded only in that build's generated CUDA-provider flags. Full CUDA compute kernels/cuDNN, real HIP/MIGraphX hardware, and the full test suite were not tested. The focused build helper and runner are local validation artifacts, not committed repository tooling; the same four tests can be selected from the normal
onnxruntime_test_allbinary.Motivation and Context
Since 1.26, thread-affine pooling retains roughly 9.7 MiB of GPU memory per calling thread per session—even when runs never overlap. Long-lived thread-pool hosts can exhaust VRAM without increasing inference concurrency. Reusing exclusively borrowed stream collections bounds retained resources by overlapping runs rather than the number of calling threads.