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

[BUG] use device_uvector instead of device_vector when possible #5380

Closed
vuule opened this issue Jun 4, 2020 · 2 comments
Closed

[BUG] use device_uvector instead of device_vector when possible #5380

vuule opened this issue Jun 4, 2020 · 2 comments
Labels
libcudf Affects libcudf (C++/CUDA) code. Performance Performance related issue

Comments

@vuule
Copy link
Contributor

vuule commented Jun 4, 2020

The new device_uvector does not initialize the allocated memory so it's much faster when the initial content is not relevant.
There are many places where device_vector can be replaced with device_uvector to improve the performance. Example:

rmm::device_vector<size_type> left_indices(left.num_rows());
thrust::sequence(
  rmm::exec_policy(stream)->on(stream), left_indices.begin(), left_indices.end(), 0);
@vuule vuule added libcudf Affects libcudf (C++/CUDA) code. Performance Performance related issue labels Jun 4, 2020
@jrhemstad
Copy link
Contributor

Furthermore, device_uvector has a stream argument in its constructor which allows correctly stream ordering the allocation. This is currently not possible with device_vector and can lead to significant issues when using non-default streams (see #2631).

@harrism harrism added this to Issue-Needs prioritizing in v0.15 Release via automation Jun 16, 2020
@harrism harrism added this to Issue-Needs prioritizing in v0.16 Release via automation Aug 10, 2020
@harrism harrism removed this from Issue-Needs prioritizing in v0.15 Release Aug 10, 2020
@harrism harrism added this to Issue-Needs prioritizing in v0.17 Release via automation Oct 1, 2020
@harrism harrism removed this from Issue-Needs prioritizing in v0.16 Release Oct 1, 2020
@harrism harrism moved this from Issue-Needs prioritizing to Issue-P0 in v0.17 Release Oct 15, 2020
@harrism harrism added this to Issue-Needs prioritizing in v0.18 Release via automation Nov 23, 2020
@harrism harrism removed this from Issue-P0 in v0.17 Release Dec 2, 2020
@harrism harrism moved this from Issue-Needs prioritizing to Issue-P1 in v0.18 Release Dec 7, 2020
@jrhemstad
Copy link
Contributor

Didn't see this when I wrote #7287. Going to close this as a duplicate since I provided a little more context in that issue.

v0.18 Release automation moved this from Issue-P1 to Done Feb 3, 2021
rapids-bot bot pushed a commit that referenced this issue Feb 4, 2021
#7256)

Small PR to provide two fixes:
- Use `rmm::device_uvector` in place of `device_vector` to improve efficiency. This is a scratch space, so supplied stream and default memory resource is used. Part of #5380
- Update `sort_helper::grouped_value` docstring to reflect change after use of stable sort.

Authors:
  - Michael Wang (@isVoid)

Approvers:
  - Vukasin Milovanovic (@vuule)
  - Ram (Ramakrishna Prabhu) (@rgsl888prabhu)
  - Mark Harris (@harrism)

URL: #7256
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libcudf Affects libcudf (C++/CUDA) code. Performance Performance related issue
Projects
No open projects
v0.18 Release
  
Done
Development

No branches or pull requests

3 participants