[NO-OOM] Exploration of cudf-spilling + UVM #1504
Draft
+81
−26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Python
|
@madsbk thank you for this! Would you be able to extract and summarize the important performance numbers? Perhaps in a graph or table to make clear the performance landscape? |
|
Some more results. I am now calling I am measuring the total time of all the joins (5 repeats each) . The timings of join repeats vary quite a bit when using UVM. This is expected since we do not reset the memory page locations between repeats.
Raw dataThe results show that, at least in this case, combining cudf-spilling with UVM clearly outperforms UVM-only without any real downside. I think the next step is to implement this in a more unintrusive way and test more workflows and hardware setups. E.g., how is the performance of cudf-spilling+UVM when running on multiple GPUs using UCX? |
harrism
reviewed
Apr 4, 2024
| @@ -203,7 +211,7 @@ class stream_ordered_memory_resource : public crtp<PoolResource>, public device_ | |||
|
|
|||
| if (size <= 0) { return nullptr; } | |||
|
|
|||
| lock_guard lock(mtx_); | |||
| // lock_guard lock(mtx_); | |||
There was a problem hiding this comment.
question: why comment out the lock?
There was a problem hiding this comment.
That is because of a deadlock that would otherwise trigger when the allocation results in cudf-spilling. In this case, cudf-spilling will find another buffer to spill and de-allocate its memory, which also
requires the lock.
I haven't given this too much thought, but I think this could be handled with a reentrant lock.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to avoid out-of-memory errors, we try to combine cudf-spilling and UVM in this PR.
Goal
The approach
Preliminary results
Running an extended version of @wence-'s join benchmark https://gist.github.com/madsbk/9589dedc45dbcfc828f7274ce3bdabc6 on a DGX-1 (32GiB).
Running with three configs:
--use-pool --base-memory-resource cuda --use-spilling--use-pool --base-memory-resource managed --use-spilling--use-pool --base-memory-resource managed --no-use-spillingTL;DR
When spilling isn’t needed, we see some overhead spikes when going from cudf-spilling to cudf-spilling+UVM and UVM-only but overall, the performance is very much on par. It might be possible to avoid these spikes using
cudaMemAdvise().When spilling is needed, the performance of cudf-spilling and cudf-spilling+UVM is very similar but again, we see some overhead spikes.
Finally, when the peak memory usage exceeds device memory, cudf-spilling fails with an OOM error. In this case, we need UVM. The performance of cudf-spilling+UVM and UVM-only are similar but cudf-spilling+UVM is more variable.
Raw Numbers
Everything fits in device memory, no spilling
cudf-spilling
cudf-spilling+UVM
UVM-only
Spilling is required
cudf-spilling
cudf-spilling+UVM
UVM-only
UVM is required
cudf-spilling
cudf-spilling+UVM
UVM-only
NB
The current code is hacky. If this approach is viable, we should implement the functionality in a standalone rmm resource.