Skip to content

Add event timing - #481

Merged
leofang merged 5 commits into
NVIDIA:mainfrom
leofang:add_timing
Mar 6, 2025
Merged

Add event timing#481
leofang merged 5 commits into
NVIDIA:mainfrom
leofang:add_timing

Conversation

@leofang

@leofangleofang commented Mar 3, 2025

Copy link
Copy Markdown
Member

When working on #475 (68c9365) I noticed we surprisingly haven't implemented event-based timing... Looks like an oversight to me because we did spend a lot of time discussing the default of the enabling_timing option and how to retrieve the timing result. This PR adds the implementation, which is also aligned with the cuda::experimental::timed_event treatment:
https://github.com/NVIDIA/cccl/blob/096596b3580d6cb2042fff692e8162d2a0ea356b/cudax/include/cuda/experimental/__event/timed_event.cuh#L90

@leofangleofang added P0 High priority - Must do! feature New feature or request cuda.core Everything related to the cuda.core module labels Mar 3, 2025
@leofangleofang added this to the cuda.core beta 3 milestone Mar 3, 2025
@leofang
leofang requested review from rwgk and vzhurba01March 3, 2025 01:02
@leofangleofang self-assigned this Mar 3, 2025
@copy-pr-bot

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@leofang

Copy link
Copy Markdown
MemberAuthor

/ok to test

@github-actions

This comment has been minimized.

@leofang

Copy link
Copy Markdown
MemberAuthor

/ok to test

@carterboxcarterbox left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the semantics of this implementation. It wouldn't have thought that you could directly subtract two Events to get the difference between them in time, but I like it.

One thing that may be missing is documentation of what happens if timing is not enabled. What is the expected behavior? I suspect the answer is that the difference is zero, but raising an error because we tried to do timing without enabling timing seems more pythonic.

Comment threadcuda_core/cuda/core/experimental/_event.py Outdated
rwgk
rwgk previously approved these changes Mar 3, 2025
Comment threadcuda_core/cuda/core/experimental/_event.py Outdated
Comment threadcuda_core/cuda/core/experimental/_event.py Outdated
Comment threadcuda_core/tests/test_event.py Outdated
Comment threadcuda_core/tests/test_event.py Outdated
@leofang

Copy link
Copy Markdown
MemberAuthor

One thing that may be missing is documentation of what happens if timing is not enabled. What is the expected behavior? I suspect the answer is that the difference is zero, but raising an error because we tried to do timing without enabling timing seems more pythonic.

Currently we do raise an exception since the driver API would return an error if the events were created without timing enabled:

cuda.core.experimental._utils.CUDAError: CUDA_ERROR_INVALID_HANDLE: invalid resource handle

So the question is if we want to do an exception chaining and raise, say, RuntimeError with a clearer err msg. @rwgk thoughts?

@leofang

Copy link
Copy Markdown
MemberAuthor

Currently we do raise an exception since the driver API would return an error if the events were created without timing enabled:

We ensure this exception is raised in commit 8aca468. The discussion for an optional exception chaining is still open.

@carterbox

carterbox commented Mar 4, 2025

Copy link
Copy Markdown
Contributor

If it's possible to catch and reraise the error with a message like "Timing must be enabled in order subtract Events. Events have timing disabled by default." That would be better than "invalid resource handle".

rwgk
rwgk previously approved these changes Mar 4, 2025
Comment threadcuda_core/cuda/core/experimental/_event.py Outdated
@leofang

Copy link
Copy Markdown
MemberAuthor

/ok to test

@leofang

Copy link
Copy Markdown
MemberAuthor

This is ready.

rwgk
rwgk approved these changes Mar 6, 2025

@rwgkrwgk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seeing the code now, I like the exception chaining approach better than the idea I posted before.

@leofang
leofang merged commit 74cd7ae into NVIDIA:mainMar 6, 2025
@leofang
leofang deleted the add_timing branch March 6, 2025 04:25
@leofang

Copy link
Copy Markdown
MemberAuthor

Thanks, Ralf/Daniel!

@github-actions

Copy link
Copy Markdown
Doc Preview CI
Preview removed because the pull request was closed or merged.

@leofangleofang mentioned this pull request Mar 6, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda.coreEverything related to the cuda.core modulefeatureNew feature or requestP0High priority - Must do!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@leofang@carterbox@rwgk