Skip to content

Implement double-buffer for ellpack.#11465

Merged
trivialfis merged 21 commits into
dmlc:masterfrom
trivialfis:ext-mixed-pages-1
May 17, 2025
Merged

Implement double-buffer for ellpack.#11465
trivialfis merged 21 commits into
dmlc:masterfrom
trivialfis:ext-mixed-pages-1

Conversation

@trivialfis

@trivialfis trivialfis commented May 15, 2025

Copy link
Copy Markdown
Member

This helps us to have the external memory cache in two different places.

  • Make the accessor a variant type.
  • Most of the changes are for calling a visit function instead of actual code changes.

todos:

  • Test the new iterator.
  • Test for external memory.
  • Alignment.
  • Profiling.

@trivialfis
trivialfis requested a review from Copilot May 16, 2025 09:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a double-buffer design for Ellpack pages so that external memory can leverage distinct host and device caches with a variant‐based accessor. Key changes include refactoring all direct accessor calls to use a visitor pattern (via Visit/VisitOnHost), updating test files to call GetHostEllpack/GetDeviceEllpack, and introducing a double buffer accessor (DoubleEllpackAccessor).

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/cpp/* Updated testing code to use Visit and GetHostEllpack
src/data/ellpack_page.cuh & related files Introduced variant-based accessor and double-buffer logic
src/tree/updater_gpu_hist.cu Updated accessor calls via lambdas using the new API
src/common/compressed_iterator.h Extended compressed iterator to support double buffering
include/xgboost/base.h Updated copyright years and minor style adjustments
Comments suppressed due to low confidence (2)

src/data/ellpack_page.cuh:325

  • [nitpick] Consider adding more detailed documentation for the Visit and VisitOnHost methods to clarify the expected callback signature and how both accessor variants are handled.
template <typename Fn> decltype(auto) Visit(Context const* ctx, common::Span<FeatureType const> feature_types, Fn&& fn) const {

src/data/ellpack_page_source.cu:249

  • There appears to be an extra semicolon after the call to dst->NullValue(), which can be removed for cleaner code.
auto null = dst->NullValue();;

Comment thread src/common/compressed_iterator.h Outdated
@trivialfis trivialfis changed the title [WIP] Implement double-buffer for ellpack. Implement double-buffer for ellpack. May 16, 2025
@trivialfis
trivialfis marked this pull request as ready for review May 16, 2025 19:50
@trivialfis

Copy link
Copy Markdown
Member Author

cc @rongou

@trivialfis
trivialfis merged commit 94bb1da into dmlc:master May 17, 2025
@trivialfis
trivialfis deleted the ext-mixed-pages-1 branch May 17, 2025 06:29
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.

3 participants