Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW] Explicit stream argument for device_buffer methods #169

Merged
merged 4 commits into from
Oct 30, 2019

Conversation

harrism
Copy link
Member

@harrism harrism commented Oct 29, 2019

When reviewing #167 I realized that some of the complexity of that PR is a result of rmm::device_buffer() having a reference to a stream that it uses internally. This raised some concerns in my mind.

  1. What if the stream no longer exists when rmm::device_buffer calls cudaMemcpyAsync in its copy constructor, resize, or shrink_to_fit? Error.
  2. Possibly more important. If someone uses one of these operators, it’s not clear externally that it is asynchronous. If they then use the device buffer’s memory on a different stream without first syncing its internal stream, there’s a race condition. It’s not obvious that the user needs to call device_buffer::stream() to get that stream and then sync it.

I think it's more obvious for device_buffer asynchronous methods to take an explicit stream argument. (The exception is operator=, since it can't take a stream parameter).

This PR makes that change. Now, the stored _stream is only used by the destructor. To enable changing what stream is used for destruction, this PR also adds a new set_stream() method.

@harrism harrism added the 3 - Ready for review Ready for review by team label Oct 29, 2019
@harrism harrism requested a review from jrhemstad October 29, 2019 04:25
@harrism harrism self-assigned this Oct 29, 2019
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be tested with cuDF before merging.

include/rmm/device_buffer.hpp Show resolved Hide resolved
include/rmm/device_buffer.hpp Outdated Show resolved Hide resolved
@harrism
Copy link
Member Author

harrism commented Oct 29, 2019

@jrhemstad I tested cudf locally before I opened the PR. I will create a same-name branch PR in cuDF to do CI tests before merging.

@jrhemstad jrhemstad merged commit 7773650 into rapidsai:branch-0.11 Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for review Ready for review by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants