Skip to content
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

Batch UpdatePosition calls #7925

Closed
wants to merge 38 commits into from

Conversation

RAMitchell
Copy link
Member

@RAMitchell RAMitchell commented May 20, 2022

Also removes position information from inside RowPartitioner class. We don't actually need it anywhere.

FinalisePosition has been moved outside the class, and now calculates positions as well as leaf values, which can be used for UpdatePredictionCache.

I have removed prediction caching for external memory in gpu_hist.

@RAMitchell
Copy link
Member Author

Some thoughts on further re-factoring:

  • Updaters should not see the task. Their behaviour should not depend on the objective function.
  • The prediction cache update is perhaps best returned from the tree->Update call alongside position information. It is very easy to calculate the prediction when you are calculating the row positions. Alternatively GBTree can just calculate this using the positions obtained from the update.
  • Our lives would be made much easier if every updater consistently supported position information and prediction caching, without exceptions.
  • The gradient based sampling needs to transparently show which samples it has selected to enable the above for gpu_hist.

@RAMitchell
Copy link
Member Author

Single GPU benchmarks, max_depth=20

dataset master batch-position-scan
airline 5014.992213 4659.965927
bosch 94.40516737 92.6324375
covtype 190.9750907 162.9997489
epsilon 986.6671753 979.8350237
fraud 1.618839191 1.577903566
higgs 2577.564037 2177.161882
year 2251.341695 2081.802971

@RAMitchell
Copy link
Member Author

There is a performance regression for airline dataset (many rows) with max_depth=8. One of the batched kernels shows poor performance and needs to be profiled/optimised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant