Add a file-backed DiskBuffer - #1195
Draft
nirandaperera wants to merge 2 commits into
Draft
Conversation
Signed-off-by: niranda perera <niranda.perera@gmail.com>
nirandaperera
added
improvement
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
nirandaperera
changed the title
DiskBuffer handleDiskBuffer
This was referenced Sep 3, 2026
pentschev
reviewed
Sep 7, 2026
Comment on lines
+38
to
+39
| * Transfers ownership of the backing file. The moved-from object is empty | ||
| * (`path()` is empty and `size()` is zero). |
Member
There was a problem hiding this comment.
Suggested change
| * Transfers ownership of the backing file. The moved-from object is empty | |
| * (`path()` is empty and `size()` is zero). | |
| * Transfers ownership of the backing file. |
Member
There was a problem hiding this comment.
Seems unnecessarily verbose docstring for a mover.
Comment on lines
+103
to
+104
| * @param reservation Memory reservation covering at least `source->size()` | ||
| * bytes. |
Member
There was a problem hiding this comment.
Why at least, and not exactly?
Comment on lines
+61
to
+65
| RAPIDSMPF_EXPECTS( | ||
| source->is_latest_write_done(), | ||
| "cannot write buffer to disk with pending stream-ordered writes", | ||
| std::logic_error | ||
| ); |
Member
There was a problem hiding this comment.
Could we validate source->is_latest_write_done() before calling create_unique_path()? As written, a buffer with pending stream-ordered writes throws here after path has already been created, and thus not cleaned.
Comment on lines
+55
to
+58
| * @note This is a point-in-time check and is subject to TOCTOU races: | ||
| * another thread may call `get()` after this returns `true`. Like | ||
| * `std::future`, concurrent `is_ready()`/`get()` from multiple | ||
| * threads is undefined behavior. |
Member
There was a problem hiding this comment.
Suggested change
| * @note This is a point-in-time check and is subject to TOCTOU races: | |
| * another thread may call `get()` after this returns `true`. Like | |
| * `std::future`, concurrent `is_ready()`/`get()` from multiple | |
| * threads is undefined behavior. | |
| * @note This is a point-in-time check and is subject to TOCTOU races: | |
| * another thread may call `get()` after this returns `true`. Like | |
| * `std::future`, concurrent `is_ready()`/`get()` from multiple | |
| * threads is undefined behavior. |
Comment on lines
+66
to
+70
| * @return Number of bytes transferred. The caller must check this against | ||
| * the requested size. | ||
| * | ||
| * @note Like `std::future::get()`, this is not thread-safe. Calling get() | ||
| * concurrently from multiple threads is undefined behavior. |
Member
There was a problem hiding this comment.
Suggested change
| * @return Number of bytes transferred. The caller must check this against | |
| * the requested size. | |
| * | |
| * @note Like `std::future::get()`, this is not thread-safe. Calling get() | |
| * concurrently from multiple threads is undefined behavior. | |
| * @return Number of bytes transferred. The caller must check this against | |
| * the requested size. | |
| * | |
| * @note Like `std::future::get()`, this is not thread-safe. Calling get() | |
| * concurrently from multiple threads is undefined behavior. |
| * `std::future`, concurrent `is_ready()`/`get()` from multiple | ||
| * threads is undefined behavior. | ||
| */ | ||
| [[nodiscard]] virtual bool is_ready() const = 0; |
Member
There was a problem hiding this comment.
This seems to have been added by this PR (not included in #1186 that introduced this class) and is only used by DiskFutureIsReadyBeforeGet, do we really need it just to satisfy testing?
Comment on lines
+56
to
+58
| RAPIDSMPF_EXPECTS( | ||
| disk_resource_ != nullptr, "the disk resource pointer cannot be NULL" | ||
| ); |
Member
There was a problem hiding this comment.
Why not? Isn't that effectively forcing us to enable disk spilling, even when that's not wanted?
Comment on lines
+84
to
+87
| // create a dir for each pid under the spill directory | ||
| std::shared_ptr<disk::DiskResource> disk_res{ | ||
| new disk::DiskResource{std::move(spill_directory) / std::to_string(::getpid())} | ||
| }; |
Member
There was a problem hiding this comment.
And also forcing creating a spilling directory?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DiskResourcecould read and write files, but nothing owned a backing file or moved aBufferto and from disk. This addsDiskBufferas a move-only, file-backed handle outside theMemoryTypetaxonomy, owned throughunique_ptr.from_bufferand restore withrestore; both block until the transfer finishes and delete the file when the handle is released.BufferResourceown ashared_ptr<DiskResource>so disk handles can outlive the buffer resource, and allocate unique spill paths under its directory.Closes #1184
Depends on #1193 #1186
Related #1170