-
Notifications
You must be signed in to change notification settings - Fork 791
[SYCL][Doc] Add spec to record an event #20309
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
base: sycl
Are you sure you want to change the base?
Conversation
Add a proposed extension specification which allows an application to reuse the same event object in multiple command submissions, rather than creating a new event for each submission.
This PR proposes a new extension that relies on changes in #20308. However, I think the two PRs can be approved and merged separately. Note that this PR (#20309) only proposes a new extension, so it's OK if this proposed extension is merged even before #20308 is implemented. It is only necessary that #20308 be implemented by the time the extension proposed in #20309 is implemented. |
This extension adds the ability to reuse the same `event` object in multiple | ||
command submissions, rather than creating a new event for each submission. | ||
This pattern may perform better on some implementations because fewer event | ||
objects need to be created and destroyed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergey-semenov are you familiar with the way events are managed in the DPC++ SYCL runtime? If so, I'd be interested in your thoughts on whether this proposed API would allow us to implement events more efficiently. I have an intuition that it will help, but I don't know how the code really works today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we're not doing anything fancy with events. it's all very straightforward with no reuse of memory whatsoever. This extension would introduce such memory reusage, but we could also just implement a memory pool for events internally, so it shouldn't be the main point here.
This should allow us to get rid of UR event creation/release though, assuming UR and its adapters can provide and take advantage of enqueue API that reuses a UR event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider counter based event? If not, the re-record will be hard to consider.
This extension adds the ability to reuse the same `event` object in multiple | ||
command submissions, rather than creating a new event for each submission. | ||
This pattern may perform better on some implementations because fewer event | ||
objects need to be created and destroyed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we're not doing anything fancy with events. it's all very straightforward with no reuse of memory whatsoever. This extension would introduce such memory reusage, but we could also just implement a memory pool for events internally, so it shouldn't be the main point here.
This should allow us to get rid of UR event creation/release though, assuming UR and its adapters can provide and take advantage of enqueue API that reuses a UR event.
* If a recorded event is used as a command dependency for some other command | ||
_C2_ (e.g. via `handler::depends_on`), the dependency is captured at the point | ||
when _C2_ is submitted. | ||
The dependency does _not_ change if the event is subsequently overwritten via | ||
`record_event`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that supposed to work in scenarios where C2 submission to UR is delayed on the SYCL runtime side (current implementation of host tasks, for example)?
I think technically we could create a "proxy" event to capture the "state" of the original event at the point of C2 submission to SYCL. But then in order to support this extension, we would have to do that any time a command is delayed, which seems problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would not be a problem. The API allows the runtime to reuse the backend event, but it does not mandate this. Therefore, if there are extenuating circumstances in some cases, the runtime is still allowed to create a new backend event if it wants.
In the case you describe, couldn't the runtime do this:
- Drop the reference to the existing backend event. This will get cleaned up however we currently clean up backend events.
- Create a new backend event and associate it with the SYCL event the user passes in.
- When the runtime eventually submits command C2 to the backend, it uses the backend event it created above in step 1.
We'll have to do the same sort of thing in other cases too. For example, consider the case when the event passed in via record_event
is from a different backend than the new command being submitted. In this case also, the runtime will need to drop the reference to the old backend event and create a new one. This is the situation I describe below under "Implementation notes".
Just catching up with the discussion, reusable event might need counter-based event. Or at least very straightforward with counter-based event. So, when the backend feature is available we should use it. |
Add a proposed extension specification which allows an application to reuse the same event object in multiple command submissions, rather than creating a new event for each submission.