add ellpack page #4833

rongou · 2019-09-04T22:15:00Z

@RAMitchell @trivialfis @sriramch this PR pulls out the quantile sketches and binning compression into a separate EllpackPage. For now the whole ELLPackMatrix is still kept in GPU memory. Next step is to break apart the matrix and persist to disk.

Please take a look.

Part of #4357.

rongou · 2019-09-12T15:49:04Z

@hcho3 Have you ever seen this error in the Travis python test?

E               urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1076)>

rongou · 2019-09-12T17:16:08Z

@hcho3 Never mind. Seems to have fixed itself.

trivialfis

Looks good to me overall. One issue though, could you please help adding null tests? I hope that XGBoost can deal with empty dataset one day ...

src/common/hist_util.cu

trivialfis · 2019-09-13T04:30:06Z

src/data/ellpack_page.cc

+class EllpackPageImpl {};
+
+EllpackPage::EllpackPage(DMatrix* dmat) {
+  LOG(FATAL) << "Not implemented.";


It's a personal preference. I would go with something more verbose like "Internal Error: XGBoost is not compiled with CUDA but EllpackPage is required" ...

trivialfis · 2019-09-13T04:30:58Z

src/data/ellpack_page.cu

+EllpackPageImpl::EllpackPageImpl(DMatrix* dmat) : dmat_{dmat} {}
+
+// Bin each input data entry, store the bin indices in compressed form.
+template<typename std::enable_if<true,  int>::type = 0>


Is there any logic change in this file?

Not the core logic, just some of the data handling.

trivialfis · 2019-09-13T04:31:31Z

src/data/ellpack_page.cuh

@@ -0,0 +1,203 @@
+/*!


And this one ?

sriramch

it may be useful to run a quick performance test with some large data corpus just to make sure there aren't perf. regressions with this.

sriramch · 2019-09-13T15:34:53Z

src/data/sparse_page_dmatrix.cc

 namespace xgboost {
 namespace data {

+extern template class SimpleBatchIteratorImpl<EllpackPage>;


why was this required? doesn't the include in line 13 specialize this meta type with ellpackpage used later?

This is forcing the compiler to not instantiate the template since we know it's already done in simple_dmatrix. Perhaps a bit over optimization. Removed.

sriramch · 2019-09-13T15:35:15Z

src/tree/updater_gpu_hist.cu

              uint32_t n_features)
      : device_id(_device_id),
+        page(page),


perhaps prefix the formal argument page with an underscore _page

sriramch · 2019-09-13T15:50:28Z

include/xgboost/data.h

@@ -287,10 +289,23 @@ class SortedCSCPage : public SparsePage {
  explicit SortedCSCPage(SparsePage page) : SparsePage(std::move(page)) {}
 };

+class EllpackPageImpl;
+class EllpackPage {


should this be more generalized so we can support multiple binned matrix types fairly easily. perhaps, make this an adapter, with a factory method to create the the underlying implementation:

struct binned_page { // common interface applicable to all pages virtual int num_bins() = 0; virtual int get_feature_bin(int bidx) = 0; virtual float get_feature_value(int fbin) = 0; // specifics for a page virtual float get_feature_value(ridx, fidx); // etc... }; struct ellpack_page : binned_page { // realize interface }; struct csr_page : binned_page { // realize interface }; // etc... static binned_page *create_binned_page(pertinent_data_after_quantile_generation);

We can probably wait until we have another binned page.

Agree, let's keep this in mind but not for this PR.

rongou

@sriramch I'll do some benchmarking.

rongou · 2019-09-13T21:28:43Z

include/xgboost/data.h

@@ -287,10 +289,23 @@ class SortedCSCPage : public SparsePage {
  explicit SortedCSCPage(SparsePage page) : SparsePage(std::move(page)) {}
 };

+class EllpackPageImpl;
+class EllpackPage {


We can probably wait until we have another binned page.

src/common/hist_util.cu

rongou · 2019-09-13T21:29:28Z

src/data/ellpack_page.cc

+class EllpackPageImpl {};
+
+EllpackPage::EllpackPage(DMatrix* dmat) {
+  LOG(FATAL) << "Not implemented.";


rongou · 2019-09-13T21:30:07Z

src/data/ellpack_page.cu

+EllpackPageImpl::EllpackPageImpl(DMatrix* dmat) : dmat_{dmat} {}
+
+// Bin each input data entry, store the bin indices in compressed form.
+template<typename std::enable_if<true,  int>::type = 0>


Not the core logic, just some of the data handling.

rongou · 2019-09-13T21:30:17Z

src/data/ellpack_page.cuh

@@ -0,0 +1,203 @@
+/*!


rongou · 2019-09-13T21:35:28Z

src/data/sparse_page_dmatrix.cc

 namespace xgboost {
 namespace data {

+extern template class SimpleBatchIteratorImpl<EllpackPage>;


This is forcing the compiler to not instantiate the template since we know it's already done in simple_dmatrix. Perhaps a bit over optimization. Removed.

rongou · 2019-09-13T21:35:44Z

src/tree/updater_gpu_hist.cu

              uint32_t n_features)
      : device_id(_device_id),
+        page(page),


RAMitchell

Looks really nice. Great to see parts of our monolithic updater_gpu_hist.cu code being extracted and becoming modular. There will be lots of opportunity to experiment with different matrix types in future.

RAMitchell · 2019-09-15T22:09:01Z

include/xgboost/data.h

@@ -287,10 +289,23 @@ class SortedCSCPage : public SparsePage {
  explicit SortedCSCPage(SparsePage page) : SparsePage(std::move(page)) {}
 };

+class EllpackPageImpl;
+class EllpackPage {


Agree, let's keep this in mind but not for this PR.

RAMitchell · 2019-09-15T22:13:43Z

src/common/hist_util.cu

@@ -148,11 +148,11 @@ struct GPUSketcher {
   public:
    DeviceShard(int device,


This DeviceShard class is now redundant right?

Yes there are 4-5 places we still have a DeviceShard that's no longer necessary. I'll send out a followup PR to clean them up.

RAMitchell · 2019-09-15T22:16:30Z

include/xgboost/data.h

@@ -287,10 +289,23 @@ class SortedCSCPage : public SparsePage {
  explicit SortedCSCPage(SparsePage page) : SparsePage(std::move(page)) {}
 };

+class EllpackPageImpl;


Can you explain briefly why this class needs pimpl?

Mainly to get CUDA-specific implementation details out of the header, since DMatrix is used all over the place in CPU-only code.

rongou · 2019-09-16T19:02:50Z

@sriramch I ran tests/benchmark/benchmark_tree.py 10 times each and got these numbers:

master: mean - 6.745 seconds, std dev - 0.063
this pr: mean - 6.720 seconds, std dev - 0.026

So the performance seems about equal.

trivialfis · 2019-09-17T03:51:06Z

@rongou Thanks!

rongou force-pushed the ellpack-page branch 10 times, most recently from a90d099 to 9578e9d Compare September 10, 2019 16:04

add ellpack page

5e81760

rongou force-pushed the ellpack-page branch from 9578e9d to 5e81760 Compare September 10, 2019 16:30

rongou added 4 commits September 10, 2019 11:17

fix r build

7fd5453

use ellpack page from gpu_hist

992d229

some refactoring and cleanup

b4cb3a9

add tests

47cc2fd

rongou added 2 commits September 12, 2019 09:01

minor fix

8b07fe3

more refactoring

f70bc6e

rongou changed the title ~~[WIP] add ellpack page~~ add ellpack page Sep 12, 2019

minor fix

82ec6ac

rongou closed this Sep 12, 2019

rongou reopened this Sep 12, 2019

trivialfis reviewed Sep 13, 2019

View reviewed changes

sriramch reviewed Sep 13, 2019

View reviewed changes

Merge branch 'master' into ellpack-page

ac6eccf

rongou commented Sep 13, 2019

View reviewed changes

address review feedback

fd464b0

RAMitchell reviewed Sep 15, 2019

View reviewed changes

add some documentaion

a3ff3dc

RAMitchell approved these changes Sep 16, 2019

View reviewed changes

trivialfis merged commit 125bcec into dmlc:master Sep 17, 2019

rongou deleted the ellpack-page branch September 19, 2019 23:09

lock bot locked as resolved and limited conversation to collaborators Dec 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ellpack page #4833

add ellpack page #4833

rongou commented Sep 4, 2019 •

edited

Loading

rongou commented Sep 12, 2019

rongou commented Sep 12, 2019

trivialfis left a comment

trivialfis Sep 13, 2019

rongou Sep 13, 2019

trivialfis Sep 13, 2019

rongou Sep 13, 2019

trivialfis Sep 13, 2019

rongou Sep 13, 2019

sriramch left a comment

sriramch Sep 13, 2019

rongou Sep 13, 2019

sriramch Sep 13, 2019

rongou Sep 13, 2019

sriramch Sep 13, 2019

rongou Sep 13, 2019

RAMitchell Sep 15, 2019

rongou left a comment

rongou Sep 13, 2019

rongou Sep 13, 2019

rongou Sep 13, 2019

rongou Sep 13, 2019

rongou Sep 13, 2019

rongou Sep 13, 2019

RAMitchell left a comment

RAMitchell Sep 15, 2019

RAMitchell Sep 15, 2019

rongou Sep 16, 2019

RAMitchell Sep 15, 2019

rongou Sep 16, 2019

rongou commented Sep 16, 2019

trivialfis commented Sep 17, 2019

		@@ -148,11 +148,11 @@ struct GPUSketcher {
		public:
		DeviceShard(int device,

add ellpack page #4833

add ellpack page #4833

Conversation

rongou commented Sep 4, 2019 • edited Loading

rongou commented Sep 12, 2019

rongou commented Sep 12, 2019

trivialfis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sriramch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RAMitchell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongou commented Sep 16, 2019

trivialfis commented Sep 17, 2019

rongou commented Sep 4, 2019 •

edited

Loading