Span class.

[trivialfis]

Implement span class from ISO++20 for safer pointer indexing.

TODOs:

• Fix convert and container constructor.
• Fix as_bytes.
• Structure tests for CUDA tests.
• Fix errors in test farm.
• Make an example use in xgboost.
• Make it work with MSVC.

[RAMitchell]

Looks very good in general. I'm wondering if we need all of the functionality here. We could possibly make this more light weight by removing the "as_bytes" functions and span vs span comparison functions. It is hard to see the use case for these in xgboost.

Although, maybe there is an argument for having a more complete implementation true to the standard.

[RAMitchell]

Duplicate check here.

[RAMitchell]

These two tests are identical.

[RAMitchell]

More duplicated tests.

[trivialfis]

@RAMitchell I will re-visit all test cases, see what's been missing.
For the as_bytes, it did came across my mind. Maybe one needs to serialize a segment of memory, like a trained model? I don't know, so obliging to the standard is just me making a safe bet.

[codecov-io]

Codecov Report

Merging #3548 into master will increase coverage by 1.94%.
The diff coverage is 88.8%.

@@             Coverage Diff              @@
##             master    #3548      +/-   ##
============================================
+ Coverage     47.85%   49.79%   +1.94%     
  Complexity      188      188              
============================================
  Files           169      172       +3     
  Lines         13330    13868     +538     
  Branches        457      457              
============================================
+ Hits           6379     6906     +527     
- Misses         6726     6737      +11     
  Partials        225      225

Impacted Files	Coverage Δ	Complexity Δ
src/common/host_device_vector.h	100% <ø> (ø)	0 <0> (ø)	⬇️
src/gbm/gblinear.cc	13.79% <0%> (-0.08%)	0 <0> (ø)
src/common/host_device_vector.cc	54.16% <0%> (-2.36%)	0 <0> (ø)
src/tree/updater_histmaker.cc	2.82% <0%> (ø)	0 <0> (ø)	⬇️
src/tree/updater_colmaker.cc	1.5% <0%> (-0.01%)	0 <0> (ø)
src/c_api/c_api.cc	23.63% <0%> (-0.05%)	0 <0> (ø)
src/tree/updater_basemaker-inl.h	0% <0%> (ø)	0 <0> (ø)	⬇️
src/tree/updater_skmaker.cc	2.16% <0%> (ø)	0 <0> (ø)	⬇️
tests/cpp/data/test_simple_csr_source.cc	100% <100%> (ø)	0 <0> (ø)	⬇️
tests/cpp/data/test_sparse_page_dmatrix.cc	96.66% <100%> (ø)	0 <0> (ø)	⬇️
... and 22 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b7a1c5...dbd61c2. Read the comment docs.

[trivialfis]

Not ready yet, seems tests is still not thorough enough.

[RAMitchell]

@hcho3 as noted by @trivialfis there is actually an array_view class in dmlc-core. After a brief look it seems it is used by the nvvm/tv projects. We could replace array_view with our span which is more powerful, better tested and compliant with the upcoming standard. What do you think?

[hcho3]

@RAMitchell You should file a pull request at dmlc-core and make a case for upgrading array_view with the span class. There seems to be a good amount of code using array_view already and it would take some work to replace all occurrences.

For the sake of lesser friction, you could simply add the span class to dmlc-core and have DMLC projects gracefully adopt it.

[trivialfis]

@RAMitchell Ready for another review. The Travis failed fetching packages in Python test, it should be fine on a good day.
EDIT: Jenkins been passed. The scary red "x" is caused by "ERROR: Queue task was cancelled" on a re-run.

The last commit is just a toy show case for using Span in kernel code. It could be reverted for a more formal integration if the rest of the code is suitable for merge.

EDIT: Later dealing with the DVec will take some time. I need to understand the whole algorithm for better integration.

[RAMitchell]

Looks good. One more thing to consider, span will incur a performance overhead when using the operator[], these changes will have worsened performance in places like this:

xgboost/src/data/simple_dmatrix.cc

Line 61 in a960391

for (bst_uint j = 0; j < inst.length; ++j) {

This can be fixed by changing this to a range based for loop, I believe this does not need to do any index checking.

Edit: Looks like you have actually already done this :)

[RAMitchell]

DVec can be dealt with later. My idea is that we can gradually enforce the usage of span across the code base using the clang-tidy linter. The most important algorithm for us is gpu_hist as it is used a lot in production, so maybe we can start there when you are ready.

I think this should be good to merge when the CI tests pass.

[trivialfis]

@RAMitchell So it passed. \0/