-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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 GPU categorical split memory allocation. #9529
Conversation
@@ -64,20 +64,13 @@ struct DeviceSplitCandidate { | |||
// split. | |||
bst_cat_t thresh{-1}; | |||
|
|||
common::CatBitField split_cats; |
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.
Trying to understand what this PR does.
Does the reference split_cats
get invalidated as more categories are added? Why is it better to get the categories from the evaluator directly?
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.
Correct. It's invalidated when more nodes are added.
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.
So same reason as why you shouldn't carry around references to vector elements? Got it.
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 think it was introduced during optimization and refactoring in 1.7. But should be fixed now. I will do some more tests myself. The error was only reproducible using a sanitizer.
Close #9521 .
split_cats
from split candidate.