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

Explicit streams in device_buffer #775

Merged

Conversation

harrism
Copy link
Member

@harrism harrism commented May 19, 2021

Removes default stream arguments from rmm::device_buffer. Now copy construction requires a stream argument (default copy ctor deleted), and copy assignment is disallowed (operator deleted). Move construction and assignment are still supported, and move assignment still use the most recently used stream for deallocating any previous data.

Also improves device_buffer tests (implements TODOs in code).

I don't think this should be merged until RAPIDS dependent libraries are ready for it. I have a libcudf PR in progress for this.

Fixes #418

@harrism harrism requested a review from a team as a code owner May 19, 2021 03:17
@harrism harrism requested review from rongou and codereport May 19, 2021 03:17
@github-actions github-actions bot added the cpp Pertains to C++ code label May 19, 2021
@harrism harrism requested a review from jrhemstad May 19, 2021 04:10
@harrism harrism added breaking Breaking change improvement Improvement / enhancement to an existing function labels May 19, 2021
@harrism harrism self-assigned this May 19, 2021
@github-actions github-actions bot added the Python Related to RMM Python API label May 19, 2021
@harrism harrism requested a review from a team as a code owner May 19, 2021 23:02
@harrism
Copy link
Member Author

harrism commented May 20, 2021

rerun tests

@harrism
Copy link
Member Author

harrism commented May 21, 2021

cuSpatial is ready for this PR: rapidsai/cuspatial#403

@harrism
Copy link
Member Author

harrism commented May 21, 2021

rerun tests

@cwharris cwharris added 3 - Ready for review Ready for review by team and removed 3 - Ready for review Ready for review by team labels May 24, 2021
@harrism harrism added the 5 - Merge After Dependencies Depends on another PR: do not merge out of order label May 24, 2021
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request May 26, 2021
)

In preparation for rapidsai/rmm#775, this PR has many changes to ensure streams on which device_buffers are allocated and copied are explicit. As a consequence, this PR also finds and fixes many places where buffers were copied that should have been moved.  In addition, it adds copy constructors that take an optional stream to all `cudf::scalar` classes.

This can and should be merged before rapidsai/rmm#775 to ensure no breakage of the build by that PR.

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Ram (Ramakrishna Prabhu) (https://github.com/rgsl888prabhu)
  - Devavret Makkar (https://github.com/devavret)
  - Jason Lowe (https://github.com/jlowe)
  - Nghia Truong (https://github.com/ttnghia)

URL: #8280
Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@harrism harrism changed the base branch from branch-21.06 to branch-21.08 May 27, 2021 00:46
@harrism harrism added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 5 - Merge After Dependencies Depends on another PR: do not merge out of order labels May 31, 2021
@harrism
Copy link
Member Author

harrism commented Jun 1, 2021

rerun tests

@harrism
Copy link
Member Author

harrism commented Jun 2, 2021

@gpucibot merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change cpp Pertains to C++ code improvement Improvement / enhancement to an existing function Python Related to RMM Python API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Make stream arguments for device_buffer explicit
6 participants