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

[Outdated] Scaling workspace resources #2194

Closed

Conversation

achirkin
Copy link
Contributor

@achirkin achirkin commented Feb 22, 2024

Brief

Add another workspace memory resource that does not have the explicit memory limit. That is, after the change we have the following:

  1. rmm::mr::get_current_device_resource() is default for all allocations, as before. It is used for the allocations with unlimited lifetime, e.g. returned to the user.
  2. raft::get_workspace_resource() is for temporary allocations and forced to have fixed size, as before. However, it becomes smaller and should be used only for allocations, which do not scale with problem size. It defaults to a thin layer on top of the current_device_resource.
  3. raft::get_large_workspace_resource() (new) is for temporary allocations, which can scale with the problem size. Unlike workspace_resource, its size is not fixed. By default, it points to the current_device_resource, but the user can set it to something backed by the host memory (e.g. managed memory) to avoid OOM exceptions when there's not enough device memory left.

Problem

We have a list of issues/preference/requirements, some of which contradict others

  1. We rely on RMM to handle all allocations and we often use rmm::mr::pool_memory_resource for performance reasons (to avoid lots of cudaMalloc calls in the loops)
  2. Historically, we've used managed memory allocators as a workaround to avoid OOM errors or improve speed (by increasing batch sizes).
  3. However, the design goal is to avoid setting allocators on our own and to give the full control to the user (hence the workaround in 2 was removed).
  4. We introduced the workspace resource earlier to allow querying the available memory reliably and maximize the batch sizes accordingly (see also issue #1310). Without this, some of our batched algorithms either fail with OOM or severely underperform due to small batch sizes.
  5. However, we cannot just put all of RAFT temporary allocations into the limited workspace_resource, because some of them scale with the problem size and would inevitably fail with OOM at some point.
  6. Setting the workspace resource to the managed memory is not advisable as well for performance reasons: we have lots of small allocations in performance critical sections, so we need a pool, but a pool in the managed memory inevitably outgrows the device memory and makes the whole program slow.

Solution

I propose to split the workspace memory into two:

  1. small, fixed-size workspace for small, frequent allocations
  2. large workspace for the allocations that scale with the problem size

Notes:

  • We still leave the full control over the allocator types to the user.
  • Neither of the workspace resource should have unlimited lifetime / returned to the user. As a result, if the user sets the managed memory as the large workspace resource, the memory is guaranteed to be released after the function call.
  • We have the option to use the slow managed memory without a pool for large allocations, while still using a fast pool for small allocations.
  • We have more flexible control over which allocations are "large" and which are "small", so hopefully using the managed memory is not so bad for performance.

Add another workspace memory resource that does not have the explicit memory limit.
It should be used for large allocation; a user can set it to the host-memory-backed resource, such as managed memory, for better scaling and to avoid many OOMs.
@achirkin achirkin added enhancement New feature or request non-breaking Non-breaking change 2 - In Progress Currenty a work in progress labels Feb 22, 2024
@achirkin achirkin self-assigned this Feb 22, 2024
@github-actions github-actions bot added the cpp label Feb 22, 2024
@achirkin achirkin added feature request New feature or request and removed enhancement New feature or request labels Feb 22, 2024
@tfeher
Copy link
Contributor

tfeher commented Feb 23, 2024

Thanks Artem for proposing this solution. On one hand, it is nice to have a secondary workspace allocator to handle large allocations. I need to still think about this.

An alternative solution would be to keep a single workspace allocator, and that would provide the large allocator as a fall-back when allocating from the fast (but smaller) pool fails.

@@ -144,7 +177,7 @@ class workspace_resource_factory : public resource_factory {
// Note, the workspace does not claim all this memory from the start, so it's still usable by
// the main resource as well.
// This limit is merely an order for algorithm internals to plan the batching accordingly.
return total_size / 2;
return total_size / 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

The OOM errors we have seen with CAGRA were related to workspace pool grabbing all this place. What about limiting to a much smaller workspace size? (E.g. faiss has 1.5 GiB limit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is an option, but so far I think it's not necessary. I also think it can hurt performance a little by reducing the batch size in places like ivf_pq::search or ivf_pq::extend.

With the current proposal, ann-bench executable (as a user of raft) set these resources:

  • default - pool on top of device memory
  • limited workspace - shares the same pool with default
  • large workspace - managed memory (without pooling)

Hence the dataset/user allocations do not conflict for the same memory with the workspace (as they both use the same pool). At the same time, large temporary allocations (such as the cagra graph on device) use the managed memory and free it as soon as the algorithm finishes.

@achirkin
Copy link
Contributor Author

achirkin commented Feb 23, 2024

Thanks for joining the conversation, Tamas. I've updated the description with my rationale since you reviewed the PR.
I think, the alternative solution you propose is a viable solution short-term to avoid OOMs, but it can be taxing on the performance: imagine a large allocation at the beginning of the algorithm (e.g. training set in kmeans) takes all of the device/workspace memory; then many subsequent small allocations are forced to use a tiny remaining free fraction of the memory via managed memory interface with a large oversubscription rate. This could lead to a terrible performance while the device memory is "wasted" on an allocation that may be not often used.

@achirkin achirkin added 3 - Ready for Review and removed 2 - In Progress Currenty a work in progress labels Feb 23, 2024
@achirkin
Copy link
Contributor Author

achirkin commented Feb 26, 2024

Benchmarking update: there's a limited evidence that the update improves performance of cagra::build: I've got ~6% speedup with default parameters on DEEP-100M (which is anyway very slow and takes a lot of memory due to large default graph degrees and low ivf-pq compression).

@tfeher
Copy link
Contributor

tfeher commented Feb 26, 2024

Thanks Artem for the update! It is a nice idea to have an extra memory resource that we can use for potentially host mem backed large temporary allocations. This can be useful for systems with improved H2D interconnect, such as Grace Hopper.

@achirkin achirkin requested a review from cjnolet March 4, 2024 07:55
@achirkin achirkin changed the base branch from branch-24.04 to branch-24.06 April 4, 2024 12:14
@achirkin achirkin marked this pull request as ready for review April 26, 2024 14:03
@achirkin achirkin requested a review from a team as a code owner April 26, 2024 14:03
@achirkin achirkin requested a review from cjnolet April 26, 2024 14:03
@achirkin achirkin changed the title [Discussion] Scaling workspace resources Scaling workspace resources Apr 29, 2024
@achirkin achirkin added breaking Breaking change and removed non-breaking Non-breaking change labels Apr 29, 2024
@achirkin achirkin changed the title Scaling workspace resources [Outdated] Scaling workspace resources May 16, 2024
@achirkin achirkin added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 3 - Ready for Review labels May 16, 2024
@achirkin
Copy link
Contributor Author

achirkin commented May 16, 2024

Opened #2322 dropping the changes to neighbor methods, which are moved to cuVS. Keeping this PR open, so that we can copy those neighbor changes when cuVS is ready for them.

@cjnolet
Copy link
Member

cjnolet commented May 17, 2024

@achirkin is this ready to be closed now that you've started a new PR for this?

@achirkin
Copy link
Contributor Author

@cjnolet If you don't mind, I'd like to keep it open until we open cuVS PR with the corresponding neighbor changes.

@achirkin achirkin changed the base branch from branch-24.06 to branch-24.08 June 6, 2024 14:56
rapids-bot bot pushed a commit to rapidsai/cuvs that referenced this pull request Jun 12, 2024
Use raft's large workspace resource for large temporary allocations during ANN index build.
This is the port of rapidsai/raft#2194, which didn't make into raft before the algorithms were ported to cuVS.

Authors:
  - Artem M. Chirkin (https://github.com/achirkin)

Approvers:
  - Tamas Bela Feher (https://github.com/tfeher)

URL: #181
@achirkin
Copy link
Contributor Author

Closing this as rapidsai/cuvs#181 got merged in cuVS

@achirkin achirkin closed this Jun 13, 2024
difyrrwrzd added a commit to difyrrwrzd/cuvs that referenced this pull request Aug 10, 2024
Use raft's large workspace resource for large temporary allocations during ANN index build.
This is the port of rapidsai/raft#2194, which didn't make into raft before the algorithms were ported to cuVS.

Authors:
  - Artem M. Chirkin (https://github.com/achirkin)

Approvers:
  - Tamas Bela Feher (https://github.com/tfeher)

URL: rapidsai/cuvs#181
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - DO NOT MERGE Hold off on merging; see PR for details breaking Breaking change cpp feature request New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

4 participants