-
Notifications
You must be signed in to change notification settings - Fork 240
Dry Run Protocol #2961
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
base: main
Are you sure you want to change the base?
Dry Run Protocol #2961
Changes from all commits
99dc47f
695a8a3
42d8ad4
6db7ec8
d91a1c6
1a114f6
dec5e95
f84d9a9
44793cd
d566fe9
fc3bde6
b0ddbc8
15c07a1
1c57abb
d916b45
9d24480
7577e56
99faf68
b859894
57d4c19
694ec63
a2dd18c
45e2d49
65d4570
d86638f
d6788f6
e7bea48
2514621
49735a5
22b4048
557cc8c
cc7a4b0
e77fe2a
8922b8f
866211e
c171d84
268eb1b
5c718d6
c5ab9c4
6af142e
ece1990
3c17e3e
4dd256b
a26357d
2a90680
59c3793
3a40d22
cce4f45
fb56025
ff20962
e76bf7c
2d3f8fc
d2cf85e
e86b56d
d9a0abf
16324fb
ced0e6e
c9bf618
1acf6cf
2326061
d4ff16e
fee4b62
f1b7aca
156a437
51c0b16
c99b879
e535124
9971c71
b682d46
69543a1
5db5727
d1cf594
e47c41f
3d93b0f
e60a048
f8754d9
0f65503
969b868
217ca58
e72e872
1c4deb8
8fdf194
1a7501c
65dda3b
a710e97
3a65638
7f1210e
592b8e0
0f5641e
205cffa
ec1a794
8d56307
a9bc0af
91107c3
fa24967
8e1855d
584cac8
a0ee878
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (2019) Sandia Corporation | ||
| * SPDX-FileCopyrightText: Copyright (c) 2022-2026, NVIDIA CORPORATION. | ||
| * SPDX-FileCopyrightText: Copyright (c) 2022-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause | ||
| */ | ||
| /* | ||
|
|
@@ -127,6 +127,29 @@ class device_uvector { | |
|
|
||
| void resize(size_type size) { data_.resize(size, data_.stream()); } | ||
|
|
||
| /** | ||
| * @brief Resize the internal buffer without copying old data. | ||
| * | ||
| * Unlike resize(), this never copies old data. | ||
| * Thus, unlike in resize(), there's no point in time where the old and the new buffers are both | ||
| * alive, and the peak memory usage is lower. | ||
| * | ||
| * Unlike resize(), this deallocates the old buffer even if the new size is smaller. | ||
| * This ensures the memory is released promptly. | ||
| */ | ||
| void reallocate(size_type size) | ||
| { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although your implementation avoids peak data (old + new) when the new size is larger than the current size, it has two issues:
I would suggest to do the current implementation only when the new size is larger. When the new size is less or equal, simply call I am not sure about clearing the buffer. It adds some overhead, but it sounds safer to me.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Funny enough, both of these are pretty much intentional features. Here's a bit more context for this. By design, mdarrays and mdspans are not supposed to be resizeable at all. However, at some point we decided to implement sparse matrices using the same container policies as mdarrays. For sparse data, one needs the resize functionality to allow changing sparsity pattern. So the Back to Essentially |
||
| if (size != data_.size()) { | ||
| auto stream = data_.stream(); | ||
| auto mr = data_.memory_resource(); | ||
| // Resize and shrink rmm::device_uvector: force deallocation without copying old data | ||
| data_.resize(0, data_.stream()); | ||
| data_.shrink_to_fit(data_.stream()); | ||
| // Assign a new value after the old one is deallocated | ||
| data_ = rmm::device_uvector<T>(size, stream, mr); | ||
| } | ||
| } | ||
|
|
||
| [[nodiscard]] auto data() noexcept -> pointer { return data_.data(); } | ||
| [[nodiscard]] auto data() const noexcept -> const_pointer { return data_.data(); } | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2022-2026, NVIDIA CORPORATION. | ||
| * SPDX-FileCopyrightText: Copyright (c) 2022-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
|
|
@@ -9,6 +9,7 @@ | |
| #include <raft/core/device_container_policy.hpp> | ||
| #include <raft/core/device_mdspan.hpp> | ||
| #include <raft/core/mdarray.hpp> | ||
| #include <raft/core/resource/dry_run_flag.hpp> | ||
| #include <raft/core/resources.hpp> | ||
|
|
||
| #include <rmm/resource_ref.hpp> | ||
|
|
@@ -164,7 +165,7 @@ auto make_device_scalar(raft::resources const& handle, ElementType const& v) | |
| using policy_t = typename device_scalar<ElementType, IndexType>::container_policy_type; | ||
| policy_t policy{}; | ||
| auto scalar = device_scalar<ElementType, IndexType>{handle, extents, policy}; | ||
| scalar(0) = v; | ||
| if (!resource::get_dry_run_flag(handle)) { scalar(0) = v; } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we don't need to guard this scalar operation (same for other resources). It is lightweight to run and keep the code a little bit simpler.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The scalar is on device - in the default per-device memory resource, which is faked in dry run mode. Therefore we have to guard it (and it falls under "GPU work" category anyway). |
||
| return scalar; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't realize we were adding this as a new resource. This would make it hard to use the dry-run for pre-initializing resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that's fine! We can still push the initialized resources back to the original resources handle on destruction of the dry run resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: with #3052 , resources initialized in dry run mode will be automatically shared back with the input resources.