-
Notifications
You must be signed in to change notification settings - Fork 889
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
Fix copy assignment and the comparison operator of rmm_host_allocator
#15677
Fix copy assignment and the comparison operator of rmm_host_allocator
#15677
Conversation
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.
🔥
*/ | ||
rmm_host_allocator& operator=(rmm_host_allocator const& other) | ||
{ | ||
mr = other.mr; |
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.
I notice that the equality operator ==
also only checks mr but not stream. Seems it's intentional but not sure why. Worth double checking with @nvdbaranec
Could we set up a stream mode test for cpp/tests/utilities_tests/io_utilities_tests.cpp
? In this way, we can avoid/uncover similar issues before delivery.
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.
Yeah, this should probably be checking the stream as well. If you had two of these objects using the same underlying allocator, but different streams you definitely could not consider them identical. Stream ordering violations could absolutely cause issues downstream.
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.
added stream comparison to the operator==
…bug-allocator-copy-wrong-stream
…vuule/cudf into bug-allocator-copy-wrong-stream
rmm_host_allocator
rmm_host_allocator
/merge |
Description
Copy assignment of
rmm_host_allocator
, used inhostdevice_vector
, is missing thestream
member assignment, leading to deallocation in the default stream in the assigned-to allocator.This PR fixes this error by switching to the auto-generated special functions.
Checklist