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 support for Events #6

Closed
bheisler opened this issue Nov 25, 2018 · 6 comments
Closed

Add support for Events #6

bheisler opened this issue Nov 25, 2018 · 6 comments
Labels
Intermediate Intermediate difficulty. May require knowledge of CUDA (or reading the Driver API docs) New CUDA Feature Expose a new CUDA feature through RustaCUDA

Comments

@bheisler
Copy link
Owner

Build a safe, rusty wrapper for the Events module.

Don't expose the cuStreamBatchMemOp, cuStreamWaitValue32 or cuStreamWriteValue32 functions, as they're deprecated in version 10 anyway.

@bheisler bheisler added New CUDA Feature Expose a new CUDA feature through RustaCUDA Intermediate Intermediate difficulty. May require knowledge of CUDA (or reading the Driver API docs) labels Nov 25, 2018
@LutzCle
Copy link
Contributor

LutzCle commented Mar 4, 2019

Hi,

I've created an initial Event type for RustaCUDA (LutzCle@4ec9fde). So far it's just some code without documentation or tests. But perhaps you could take a look and comment if this is going in the right direction?

Cheers

@bheisler
Copy link
Owner Author

bheisler commented Mar 5, 2019

This looks pretty good so far. A few comments:

I did notice that many of the functions return CudaResult<&Self> - most of these have no real return value, so I'd prefer to return CudaResult<()>.

query should return a new enum - the underlying API uses the CUresult return value to indicate whether the event has been processed or not, but we should separate error-handling from the valid return values.

It might be better for elapsed_time to return a std::time::Duration but there isn't an easy way to construct one from floating-point milliseconds. I'm not sure about this; could go either way.

Naturally, tests, documentation and documentation-tests will be needed.

@LutzCle
Copy link
Contributor

LutzCle commented Mar 6, 2019

Thank you for your feedback, I appreciate it! I'll push an update in the next days.

@LutzCle
Copy link
Contributor

LutzCle commented Mar 20, 2019

See this new commit - I believe I've addressed your comments.

Concerning elapsed_time returing std::time::Duration, there exists a tracking issue for duration_float on the Rust issue tracker. This would be perfect for implementing elapsed_time. However, it's still unstable as of now. Thus, I propose we rename elapsed_time to elapsed_time_f32 (as per my commit above) and add a new function returning a Duration when the issue becomes stable.

The general idea would be to convert the f32 value using Duration::as_secs_f32, and then rescale to milliseconds using Duration.mul_f32.

One more idea would be to use session types to ensure that record has been called on the events before calling elapsed_time (i.e., fn record(e: Event) -> RecordedEvent). An open question for me is what other functions besides elapsed_time require record to be called? query and synchronize would be the obvious candidates, but the documentation isn't very specific about those.

However, care would have to be taken to ensure that record (and other function) can still be called multiple times on the same event. A possible solution would be to make Event a trait. Then again, this clearly adds complexity to the design.

What do you think?

@bheisler
Copy link
Owner Author

Yikes, thanks for your patience on this.

I'd say this is good enough to be merged. Would you mind submitting a pull request so I can merge it in?

@LutzCle LutzCle mentioned this issue May 9, 2019
@LutzCle
Copy link
Contributor

LutzCle commented May 9, 2019

Sorry that it took so long. I meant to add Stream::wait_event as well. The PR is ready now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Intermediate Intermediate difficulty. May require knowledge of CUDA (or reading the Driver API docs) New CUDA Feature Expose a new CUDA feature through RustaCUDA
Projects
None yet
Development

No branches or pull requests

2 participants