-
Notifications
You must be signed in to change notification settings - Fork 242
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 77 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
b8e7928
721135a
8971552
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 |
|---|---|---|
|
|
@@ -126,6 +126,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
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. Thank you for sharing the context with sparse matrix. I still don't understand why when the new size is less than or equal to the current size, we don't simply free the redundant memory and keep the remaining
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. The current implementation makes sense to me only when the new size is greater than the current size |
||
| 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 |
|---|---|---|
|
|
@@ -8,6 +8,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> | ||
|
|
@@ -163,7 +164,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).
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. Acknowledged. |
||
| return scalar; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,11 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2022-2024, NVIDIA CORPORATION. | ||
| * SPDX-FileCopyrightText: Copyright (c) 2022-2026, NVIDIA CORPORATION. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| #pragma once | ||
|
|
||
| #include <raft/core/interruptible.hpp> | ||
| #include <raft/core/resource/dry_run_flag.hpp> | ||
| #include <raft/core/resource/resource_types.hpp> | ||
| #include <raft/core/resources.hpp> | ||
| #include <raft/util/cudart_utils.hpp> | ||
|
|
@@ -82,13 +83,18 @@ inline void set_cuda_stream(resources const& res, rmm::cuda_stream_view stream_v | |
| */ | ||
| inline void sync_stream(const resources& res, rmm::cuda_stream_view stream) | ||
| { | ||
| if (raft::resource::get_dry_run_flag(res)) { return; } | ||
|
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. What is the motivation for guarding Your dry_run_protocol.md suggests the opposite that
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 guide says it's safe to call this function, because it is dry-run compliant (takes the raft::resources as an argument) - hence it guards from any CUDA work inside.
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. If I understand dry-run correctly, when res is in dry-run mode, the stream contains no CUDA work and contains only light-weight fake memory operations. Therefore it would be fast to sync. If user intentionally adds a CUDA/memory work to the stream without guarding, he wants the I mean this guarding on sync stream wouldn't bring any benefit but limit its usage a little.
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. It is still CUDA work and a call to cuda context, which may briefly lock. In dry run mode, nobody should ever call CUDA, that is a hard constraint. |
||
| interruptible::synchronize(stream); | ||
| } | ||
|
|
||
| /** | ||
| * @brief synchronize main stream on the resources instance | ||
| */ | ||
| inline void sync_stream(const resources& res) { sync_stream(res, get_cuda_stream(res)); } | ||
| inline void sync_stream(const resources& res) | ||
| { | ||
| if (raft::resource::get_dry_run_flag(res)) { return; } | ||
| sync_stream(res, get_cuda_stream(res)); | ||
| } | ||
|
|
||
| /** | ||
| * @} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| #pragma once | ||
|
|
||
| #include <raft/core/resource/resource_types.hpp> | ||
| #include <raft/core/resources.hpp> | ||
|
|
||
| #include <memory> | ||
|
|
||
| namespace raft::resource { | ||
|
|
||
| /** | ||
| * @defgroup dry_run_flag Dry-run flag resource | ||
| * @{ | ||
| */ | ||
|
|
||
| /** | ||
| * @brief Resource that holds a boolean dry-run flag. | ||
| * | ||
| * When the dry-run flag is set, algorithms should skip kernel execution | ||
| * and only perform allocations to measure memory usage. | ||
| */ | ||
| class dry_run_flag_resource : public resource { | ||
| public: | ||
| dry_run_flag_resource() = default; | ||
| explicit dry_run_flag_resource(bool value) : flag_(value) {} | ||
| ~dry_run_flag_resource() override = default; | ||
|
|
||
| auto get_resource() -> void* override { return &flag_; } | ||
|
|
||
| void set(bool value) { flag_ = value; } | ||
| [[nodiscard]] auto get() const -> bool { return flag_; } | ||
|
|
||
| private: | ||
| bool flag_{false}; | ||
| }; | ||
|
|
||
| /** | ||
| * @brief Factory that creates a dry_run_flag_resource. | ||
| */ | ||
| class dry_run_flag_resource_factory : public resource_factory { | ||
| public: | ||
| explicit dry_run_flag_resource_factory(bool initial_value = false) : initial_value_(initial_value) | ||
| { | ||
| } | ||
|
|
||
| auto get_resource_type() -> resource_type override { return resource_type::DRY_RUN_FLAG; } | ||
| auto make_resource() -> resource* override { return new dry_run_flag_resource(initial_value_); } | ||
|
|
||
| private: | ||
| bool initial_value_; | ||
| }; | ||
|
|
||
| /** | ||
| * @brief Get the dry-run flag from a resources handle. | ||
| * | ||
| * @param res raft resources object | ||
| * @return true if dry-run mode is active | ||
| */ | ||
| inline auto get_dry_run_flag(resources const& res) -> bool | ||
| { | ||
| if (!res.has_resource_factory(resource_type::DRY_RUN_FLAG)) { | ||
| res.add_resource_factory(std::make_shared<dry_run_flag_resource_factory>()); | ||
|
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. How about just return False here? I think we don't need to create the factory at all in dry-run-free mode.
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 argument
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. Hmm, when we instantiate a Therefore, line 65 is only executed if the input resource hasn't been added with a dry run factory before. I may overlook somewhere though. Even if I am right, this is a minor thing IMO.
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. Ah, you mean specifically remove the |
||
| } | ||
| return *res.get_resource<bool>(resource_type::DRY_RUN_FLAG); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Set the dry-run flag on a resources handle. | ||
| * | ||
| * @param res raft resources object | ||
| * @param value true to enable dry-run mode, false to disable | ||
| */ | ||
| inline void set_dry_run_flag(resources const& res, bool value) | ||
| { | ||
| if (!res.has_resource_factory(resource_type::DRY_RUN_FLAG)) { | ||
| res.add_resource_factory(std::make_shared<dry_run_flag_resource_factory>(value)); | ||
| } else { | ||
| // The resource may already be instantiated; update it directly | ||
| auto* flag = res.get_resource<bool>(resource_type::DRY_RUN_FLAG); | ||
| *flag = value; | ||
| } | ||
| } | ||
|
|
||
| /** @} */ | ||
|
|
||
| } // namespace raft::resource | ||
Uh oh!
There was an error while loading. Please reload this page.