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

Optimization/buildhist/colwisebuildhist #8233

Merged

Conversation

razdoburdin
Copy link
Contributor

@razdoburdin razdoburdin commented Sep 8, 2022

Hi,
this PR is a part of #7192. It continues the optimization of BuildHistKernel, that was started in #8218.
Here I introduce a column wise building of histogram and a dispatcher for choice between the row wise and the column wise kernels. This optimization allow to improve runtime speed for several datasets up to 2x. For performance measurements I booked the c6i.12xlarge (24 cores, hyperthreading is off) instance on on Amazon Web Services and used the benchmarks from here.

Methodology of the benchmarking is the same as in #8218. The only two datasets from the list affecting by this optimization are santander and epsilon. For this reason only them are shown in the following table:

master this PR speedup
santander 1.02E+02 4.89E+01 2.09
epsion 1.84E+02 1.03E+02 1.78

I am looking forward for your review and comments!

upg:
I have deleted the optimization related to santander dataset for simplification of the review.

@razdoburdin
Copy link
Contributor Author

razdoburdin commented Sep 12, 2022 via email

@liuwenyi2
Copy link

Hi, for performance measurements I have used the master branch in the dmlc/xgboost repo. You can easily reproduce the calculations with it. If you need any help, you are welcome! From: wenyi Liu @.> Sent: Saturday, September 10, 2022 11:34 AM To: dmlc/xgboost @.> Cc: Razdoburdin, Dmitry @.>; Author @.> Subject: Re: [dmlc/xgboost] Optimization/buildhist/colwisebuildhist (PR #8233) Hi, Your work is very interesting. I‘d like to test your optimization effect. Could you please tell me the source version of XGBoost?

much appreciated!
Since I can only compile XGboost-1.1.0 on clusterIs.
I want to reproduce your calculations with 1.1.0 to do some measurements.
I'm trying to do it.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work on optimization. Some questions in the comments. Can we omit the optimization on column sampling for now and just build the histogram for all columns?

src/common/hist_util.cc Show resolved Hide resolved
@@ -23,10 +23,12 @@ void ColumnMatrix::InitStorage(GHistIndexMatrix const& gmat, double sparse_thres
gmat.GetFeatureCounts(feature_counts.data());

// classify features
any_sparse_column_ = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any_sparse_column_ = !all_dense_column

@@ -16,6 +19,19 @@

namespace xgboost {
namespace tree {

struct ColSample {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using TrainParam directly seems to be simpiler.

size_t n_batches_{0};
// Whether XGBoost is running in distributed environment.
bool is_distributed_{false};
// Addhoch colsample threshold level
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adhoc.

const size_t n_nodes = nodes_for_explicit_hist_build.size();
CHECK_GT(n_nodes, 0);

const common::ColumnMatrix& column_matrix = gidx.Transpose();
const bool column_sampling =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain the logic here? there are missing values but no sparse column in column matrix and bytree/bylevel lesser than threshold while there's no bynode? Is this necessary?

Also, this special case seems to be difficult to test.

@razdoburdin razdoburdin marked this pull request as draft September 19, 2022 13:16
@razdoburdin razdoburdin marked this pull request as ready for review September 19, 2022 13:54
@razdoburdin
Copy link
Contributor Author

Hi, I have removed column sampling optimization for a while. Hope it will help with a review. Thanks for the suggestion!

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please try to enable some tests for each of the case? I don't want to break anything in here by accident. ;-)

@razdoburdin
Copy link
Contributor Author

Could you please try to enable some tests for each of the case? I don't want to break anything in here by accident. ;-)

As far as column wise building is just another way to calculate the same result as the row wise building, I suggest to reuse the existing tests for histogram building.
I added force_read_by_column flag to BuildHist. It allows to use disable automatic choice of building strategy and to use column wise building instead. I duplicated the related tests with the new flag, so they now test both strategies.

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.

4 participants