Skip to content

Revert #5755 #6145

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

Closed
wants to merge 2 commits into from
Closed

Revert #5755 #6145

wants to merge 2 commits into from

Conversation

hcho3
Copy link
Collaborator

@hcho3 hcho3 commented Sep 22, 2020

Temporarily revert #5755 to prevent OOM error in the e2e mortgage notebook.

@trivialfis
Copy link
Member

I'm still looking into it. Expect some results today. Reverting it might break quantile dmatrix completely.

@hcho3
Copy link
Collaborator Author

hcho3 commented Sep 22, 2020

@trivialfis At least it does not break the gtest. Let's see whether the Python tests would pass too.

@RAMitchell
Copy link
Member

The relevant PR removes the specialised copy for column wise data and instead uses this:

// Here the data is already correctly ordered and simply needs to be compacted
// to remove missing data
template <typename AdapterT>
void CopyDataToDMatrix(AdapterT* adapter, common::Span<Entry> data,
                       int device_idx, float missing,
                       common::Span<size_t> row_ptr) {
  auto& batch = adapter->Value();
  auto transform_f = [=] __device__(size_t idx) {
    const auto& e = batch.GetElement(idx);
    return Entry(e.column_idx, e.value);
  };  // NOLINT
  auto counting = thrust::make_counting_iterator(0llu);
  thrust::transform_iterator<decltype(transform_f), decltype(counting), Entry>
      transform_iter(counting, transform_f);
  dh::XGBCachingDeviceAllocator<char> alloc;
  thrust::copy_if(
      thrust::cuda::par(alloc), transform_iter, transform_iter + batch.Size(),
      thrust::device_pointer_cast(data.data()), IsValidFunctor(missing));
}

I wonder if the copy_if is responsible for the extra memory allocation.

@RAMitchell
Copy link
Member

Thrust is using n words to store predicates for the output location of copy_if: https://github.com/thrust/thrust/blob/94e0b1c63fadd5c574c45f05267e45ac136e78ef/thrust/system/detail/generic/copy_if.inl#L64

We can write this more efficiently by redirecting the output of an exclusive scan purely with iterators.

Does someone want to try this? Otherwise I can implement myself.

@trivialfis
Copy link
Member

Let me give a try.

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2020

Codecov Report

Merging #6145 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6145   +/-   ##
=======================================
  Coverage   78.83%   78.83%           
=======================================
  Files          12       12           
  Lines        3090     3090           
=======================================
  Hits         2436     2436           
  Misses        654      654           

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 33d80ff...52f4cce. Read the comment docs.

@trivialfis
Copy link
Member

I don't think this is necessary anymore. @hcho3 Thanks for bisecting the code.

@trivialfis trivialfis closed this Oct 18, 2020
@hcho3 hcho3 deleted the revert_5755 branch October 18, 2020 17:00
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