Repository navigation
feat(hipfile): Break up fallback async ios into smaller chunks reusing a single bounce buffer - #10787
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a per-stream pinned “bounce buffer” for fallback async I/O and updates the fallback async path to split large transfers into multiple chunked CPU-copy + kernel-copy steps, controlled by a new HIPFILE_ASYNC_BUFFER_SIZE environment variable.
Changes:
- Add
HIPFILE_ASYNC_BUFFER_SIZEconfiguration/environment plumbing and expose it viaConfiguration::asyncBufferSize(). - Allocate and retain a pinned async buffer in
hipFile::Stream, and plumb buffer host/dev pointers + size throughIStream. - Update fallback async I/O and memcpy kernel logic to operate on chunked transfers; adjust unit/system tests accordingly.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/hipfile/src/amd_detail/environment.h | Adds constrained unsigned-integral Environment::get overload and async buffer env var declaration. |
| projects/hipfile/src/amd_detail/environment.cpp | Removes old unsigned-int specialization and adds async_buffer_size() accessor. |
| projects/hipfile/src/amd_detail/configuration.h | Adds Configuration::asyncBufferSize() API. |
| projects/hipfile/src/amd_detail/configuration.cpp | Implements async buffer size lookup (default 1 MiB). |
| projects/hipfile/src/amd_detail/stream.h | Extends IStream with async buffer getters; adds async buffer members to Stream. |
| projects/hipfile/src/amd_detail/stream.cpp | Allocates pinned per-stream async buffer and exposes getters. |
| projects/hipfile/src/amd_detail/backend/asyncop-fallback.h | Refactors AsyncOpFallback to use stream-owned bounce buffer and track per-chunk byte counts. |
| projects/hipfile/src/amd_detail/backend/asyncop-fallback.cpp | Switches async op construction to reuse stream async buffer; removes per-op bounce allocation. |
| projects/hipfile/src/amd_detail/backend/memcpy-kernel.hip | Updates kernel to copy only a single chunk and coordinate chunk sizing via chunk_bytes_copied. |
| projects/hipfile/src/amd_detail/backend/fallback.h | Declares new async_io_advance host function. |
| projects/hipfile/src/amd_detail/backend/fallback.cpp | Chunks fallback async I/O by stream buffer size; adds per-chunk advance host function; updates cpu-copy logic. |
| projects/hipfile/src/amd_detail/async.cpp | Stops clearing op shared_ptr members in completeOp (resource lifetime change). |
| projects/hipfile/test/amd_detail/mconfiguration.h | Mocks Configuration::asyncBufferSize(). |
| projects/hipfile/test/amd_detail/mstream.h | Mocks new IStream async buffer methods. |
| projects/hipfile/test/amd_detail/configuration.cpp | Adds tests for async buffer size env var parsing/defaulting. |
| projects/hipfile/test/amd_detail/environment.cpp | Adds unsigned-integral overload coverage (contains a test-name typo). |
| projects/hipfile/test/amd_detail/stream.cpp | Updates stream tests for async buffer allocation and adds failure-path coverage. |
| projects/hipfile/test/amd_detail/async.cpp | Updates async fallback tests for stream-owned bounce buffer and chunked async behavior. |
| projects/hipfile/test/system/async.cpp | Updates system tests for chunked kernel semantics and new op fields. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
8a18e3e to
075271c
Compare
|
RE: Introduce HIPFILE_ASYNC_BUFFER_SIZE environment variable for controlling the bounce buffer size. Since we're adding a new env variable, we should document it on the env variable page. Ex: https://rocm.docs.amd.com/projects/hipFile/en/latest/reference/hipFile-environment-variables.html |
john00003
left a comment
There was a problem hiding this comment.
The Async path was unfamiliar to me, but I got to learn a ton about it (and how our mocking works) while working through this, thanks for the help with that. LGTM I commented on everything that came to my mind.
5966564 to
0df656c
Compare
…e bounce buffer in the stream.
b56f47f to
442909e
Compare
TheRock Submodule Bump ActivityNewest first
|
Motivation
The plan is to potentially lower the memory footprint of an async workload by reusing a single bounce buffer for all async IOs performed on a stream. To accommodate this the IOs may need to be broken into smaller chunks that fit on the bounce buffer.
Technical Details
Issue Tracking
JIRA ID: AIHIPFILE-235
Test Plan
Running unit tests and system tests on Rowlet.
Test Result
Tests pass.
Submission Checklist