Move buffers into decision_forest, to avoid triggering copy constructor - #196
Merged
Conversation
6 tasks
decision_forest, to avoid copying
decision_forest, to avoid copyingdecision_forest, to avoid trigger copy constructor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe constructors now move forest buffers and variants into owned members. Validation and device initialization use the transferred buffers. ChangesForest construction
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change moves rvalue buffers into the model to avoid unintended copies and preserve stream ordering. The PR is merge-ready after normal checks; updating the public ownership documentation remains a minor follow-up. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
decision_forest, to avoid trigger copy constructordecision_forest, to avoid triggering copy constructor
RAMitchell
approved these changes
Aug 13, 2026
trivialfis
approved these changes
Aug 13, 2026
rapids-bot Bot
pushed a commit
that referenced
this pull request
Sep 8, 2026
Extracted from #193 ~~Requires #196 for the CI to pass.~~ - [x] Remove RAFT and RMM from CMake config - [x] Remove the use of `raft::handle_t` and `nvforest::handle_t` and use CUDA streams directly - [x] Use `cuda::buffer` instead of `rmm::device_buffer`. Since `cuda::buffer` requires NVCC to build, we need to use PIMPL with type erasure. - [x] Fix all gtests - [x] Update the Python layer - [x] Add test coverage for using stream with wrong device. Authors: - Philip Hyunsu Cho (https://github.com/chyunsu3) Approvers: - Bradley Dice (https://github.com/bdice) - Simon Adorf (https://github.com/csadorf) URL: #195
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
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.
Currently, buffer arguments to
decision_forestare passed to member variables withoutstd::move, triggering the copy constructor. This behavior is not intended, since the arguments are all rvalues (&&).The extra buffer copies are problematic because the copy constructor uses the default stream. If the initial host-to-device copy occurs on a non-default stream, then the copy construction is unordered with respect to the host-to-device copy (i.e. may occur before or after). So the forest model may end up with garbage value.
Fix. Ensure that the buffer arguments are moved into the
decision_forest. This way, we do not make extra copies after the initial host-to-device copy.I found this bug while working on #193, since it was using a non-default stream.