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

Add HostMemoryAllocator interface #13924

Merged
merged 8 commits into from
Aug 23, 2023

Conversation

gerashegalov
Copy link
Contributor

@gerashegalov gerashegalov commented Aug 19, 2023

Description

Creates an interface to intercept calls to HostMemoryBuffer.allocate

Fixes NVIDIA/spark-rapids#8884

Signed-off-by: Gera Shegalov [email protected]

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@gerashegalov gerashegalov added the Spark Functionality that helps Spark RAPIDS label Aug 19, 2023
@gerashegalov gerashegalov self-assigned this Aug 19, 2023
@gerashegalov gerashegalov requested a review from a team as a code owner August 19, 2023 05:34
@github-actions github-actions bot added the Java Affects Java cuDF API. label Aug 19, 2023
@gerashegalov gerashegalov added 3 - Ready for Review Ready for review by team non-breaking Non-breaking change feature request New feature or request labels Aug 19, 2023
@gerashegalov gerashegalov requested a review from revans2 August 21, 2023 17:10
Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

The current set of changes look good, but how do we actually inject our own HostMemoryAllocator for something like copying data from the device to the host?

We also will need to look at the C++ code at some point.

jobject allocate_host_buffer(JNIEnv *env, jlong amount, jboolean prefer_pinned);

That is really only used for I/O operations like arrow and the writer APIs.

@gerashegalov
Copy link
Contributor Author

gerashegalov commented Aug 21, 2023

@revans2

how do we actually inject our own HostMemoryAllocator for something like copying data from the device to the host?

If we need full flexibility of initializing in the layer above cuDF/Java, and if it's a singleton I was thinking of an API like Cuda.setHostMemoryAllocator(HostMemoryAllocator hma).

We also will need to look at the C++ code at some point.

jobject allocate_host_buffer(JNIEnv *env, jlong amount, jboolean prefer_pinned);

That is really only used for I/O operations like arrow and the writer APIs.

I can add this once it's clear where the singleton allocator lives.

@revans2
Copy link
Contributor

revans2 commented Aug 21, 2023

Lets take the example of ColumnView.copyToHost, which the Spark plugin could use for columnar to row conversion. One of the rules we need to follow to avoid deadlocks is that all host memory is spillable if we do a blocking allocation call. But because ColumnView.copyToHost can allocate multiple buffers before returning we cannot make them spillable in between each allocation without even more major changes to copyToHost. The plan to work around this was to ask the ColumnView how much memory we needed to allocate (#13919) and then reserve that amount so we can be guaranteed that the memory is available for us to allocate.

I see two ways to do this.

  1. Change every single API that allocates large amounts of host memory to take an HostMemoryAllocator as a parameter.
  2. Have a singleton, but the singleton we put in would have enough smarts that the plugin could call into it to say for this block of code allocate memory from this reservation instead.

Originally I thought we would do option 1, but the singleton idea is growing on me, and I think we could have the APIs be just as clean. If you do want to do a singleton that is fine. I will just need to make sure that we have an issue to implement a HostMemoryAllocator that would give us the flexibility we need.

@gerashegalov
Copy link
Contributor Author

I added explicit HostMemoryAllocator parameter passing because it is indeed simple and gives us most flexibility.

still need to think about allocate_host_buffer, maybe in a separate PR after all.

@gerashegalov gerashegalov requested a review from revans2 August 22, 2023 20:59
Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Looks good. If we want to have a follow on for the native code that is fine.

@gerashegalov
Copy link
Contributor Author

/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request Java Affects Java cuDF API. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Provide CUDF java APIs for memory allocation
2 participants