[ 🚧 Draft] : Adding host-mr for pinned bounce buffer to rmm::device_scalar
#1985
Conversation
Signed-off-by: Jigao Luo <[email protected]>
JigaoLuo
changed the title
rmm::device_scalarrmm::device_scalar
JigaoLuo
changed the title
rmm::device_scalarrmm::device_scalar
JigaoLuo
changed the title
rmm::device_scalarrmm::device_scalar
| } | ||
| } | ||
|
|
||
| // Disallow passing literals to set_value to avoid race conditions where the memory holding the |
There was a problem hiding this comment.
I believe you can do this too now? copy to bounce buffer immediately and then do the copy async
There was a problem hiding this comment.
Hi @devavret , thanks!
Sorry. I wasn’t sure I understood you correctly.
These 3 lines refer to the else case, where no host-pinned bounce buffer is allocated. That’s actually the situation we’re currently only having in RMM.
Do you mean we just allocate a buffer for every function call of this?
There was a problem hiding this comment.
I think @devavret is referring to the deleted function below.
There was a problem hiding this comment.
I get it now. Let’s first discuss where the buffer should be placed, as mentioned above. Once that’s settled, I can address this detailed point.
| return **_host_bounce_buffer; | ||
| } else { | ||
| // Case: Copying with pageable host memory — may trigger an implicit synchronization. | ||
| return _storage.front_element(stream); |
There was a problem hiding this comment.
This makes me wonder if the bounce buffer copying support should be in device_buffer instead
There was a problem hiding this comment.
Hi @harrism, thanks! Yes, that’s also my main concern. It’s why I didn’t prepare a well-considered pull request but only a draft here, as there’s a chance it’ll need to be rewritten again, just like I already did in cuDF.
We can discuss where the buffer should live. cuDF places the buffer in its scalar header, and that’s the behavior I’d like to mimic.
Alternatively, storing it in the RMM devicevector header is also possible, since the element call results in a copy as well. The twin issue is here: #1955
| } | ||
| } | ||
|
|
||
| // Disallow passing literals to set_value to avoid race conditions where the memory holding the |
There was a problem hiding this comment.
I think @devavret is referring to the deleted function below.
Hi @bdice |
JigaoLuo
force-pushed
the
adding-buffer-device_scalar
branch
from
9e10849 to
ad09350
Compare
|
Hi all reviewers, I’d like to close this draft and move to a new one #1996, where I’ll be rewriting |
4 participants
Description
This is an initial draft for Issue #1959 that adds a host memory resource for a pinned bounce buffer into
rmm::device_scalar.It’s an early attempt and still incomplete, particularly in the number of constructors and unit tests. I expect this PR draft will spark some discussion, and I subscribe to the idea that “perfect is the enemy of good.” I hope it's okay to use this draft as a starting point for conversation. If any major changes are needed, I’m happy to revisit or even drop the current approach for a better one.
Checklist