-
-
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
Re-introduce double buffer in UpdatePosition, to fix perf regression in gpu_hist #6757
Conversation
This reverts commit f779980.
dh::TemporaryArray<bst_node_t> position_temp(position_a_.size()); | ||
dh::TemporaryArray<RowIndexT> ridx_temp(ridx_a_.size()); |
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 suspect building TemporaryArray
at every invocation of UpdatePosition
gets expensive in the particular example provided.
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.
Could you please share some benchmark results?
@trivialfis The posted example in #6552 won't complete in a reasonable amount of time with the latest master (and 1.3.0). I can leave it up over night and see if it finishes by tomorrow. On the other hand, here's the result with the proposed fix:
|
Thanks for sharing, what about more conventional dataset? |
Nah, no need. I'm just worrying about there might be regression on other types of data. |
Any suggestion? Should I try gbm-bench? |
That would be great! |
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.
Looks good as long as there's no regression on other datasets.
@trivialfis Here is the benchmark results on gbm-bench. I do not see any performance degradation:
|
Closes #6552 by partially reverting the commit f779980