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

[FEA] Method to copy device_buffer to host #216

Closed
jakirkham opened this issue Dec 17, 2019 · 9 comments · Fixed by #219
Closed

[FEA] Method to copy device_buffer to host #216

jakirkham opened this issue Dec 17, 2019 · 9 comments · Fixed by #219
Labels
doc Documentation

Comments

@jakirkham
Copy link
Member

Is your feature request related to a problem? Please describe.

Currently device_buffer is able to construct itself using memory on the host. It would be useful to do the opposite. Namely have a method to copy data back to the host in some buffer.

Describe the solution you'd like

A method like copy_to_host that returns something (maybe std::string?) that has a copy of the data from the device_buffer now on the host.

Describe alternatives you've considered

Users could probably do this themselves using device_buffer directly.

Additional context

This would be helpful not only at the C++ level, but also in Python where we would want an analogous feature (maybe tobytes?).

cc @kkraus14 @shwina

@jrhemstad
Copy link
Contributor

jrhemstad commented Dec 19, 2019

The biggest impediment to this would be what would this return? std::string isn't really appropriate. Neither is std::vector<char>. So it probably requires creating a new class in RMM like host_buffer, which opens a sizable can of worms, similar to the issues I described here: #141 (comment)

rmm::host_buffer is certainly possible, it would just require quite a bit of new supporting infrastructure, which would likely require redesign/refactoring of existing infrastructure.

@harrism
Copy link
Member

harrism commented Dec 19, 2019

Well, the expedient thing would be to add a method like:

device_buffer::copy_to_host( void* host_buffer);

It's a kind of halfway-between-C-and-C++ solution that basically is just a wrapper around cudaMemcpy. The user is responsible for allocating and owning host_buffer to the size returned by device_buffer::size().

@jakirkham
Copy link
Member Author

If it's non-trivial to add in the C++ layer, we can workaround its absence in the Python layer. Just would be nice to have a plan for alignment so they don't get too far apart.

@harrism
Copy link
Member

harrism commented Dec 19, 2019

Look, I'm OK with adding my suggested API (which is trivial) because it is congruent with the constructor that takes a void pointer (as you mentioned in the initial request, @jakirkham ). But I want to give @jrhemstad the opportunity to oppose that approach if he has a strong opinion.

@jrhemstad
Copy link
Contributor

Look, I'm OK with adding my suggested API (which is trivial) because it is congruent with the constructor that takes a void pointer (as you mentioned in the initial request, @jakirkham ). But I want to give @jrhemstad the opportunity to oppose that approach if he has a strong opinion.

I have no problem with a free function that looks like:

void to_host(rmm::device_buffer const& b, void * host_buffer);

It would be a pretty minimal wrapper around cudaMemcpy, but if people think that's useful, then I've no quarrel with that.

@jakirkham
Copy link
Member Author

That would be great. Would it be possible for it to perform error handling as well?

@jrhemstad
Copy link
Contributor

That would be great. Would it be possible for it to perform error handling as well?

Yes, it can throw an exception if host_buffer is null or if the cudaMemcpy fails.

@harrism
Copy link
Member

harrism commented Dec 20, 2019

@jakirkham how about submitting a PR? :)

@jakirkham
Copy link
Member Author

Happy to. Submitted PR ( #219 ) to start the conversation.

Am a bit rusty on my C++, so advice welcome there. In particular could use some help on figuring out where tests should go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants