Skip to content

Conversation

@slawekptak
Copy link
Contributor

Due to an inefficiency in how the event-related functions are called, there is overhead even if the events vector is empty. This is a temporary fix, to avoid the call if not needed.

@slawekptak slawekptak requested a review from a team as a code owner September 23, 2025 10:25
@slawekptak slawekptak requested a review from againull September 23, 2025 10:25
@slawekptak slawekptak requested a review from vinser52 September 23, 2025 10:25
Due to an inefficiency in how the event-related functions
are called, there is overhead even if the events vector is empty.
This is a temporary fix, to avoid the call if not needed.
Comment on lines +506 to +507
// TODO checking the size of the events vector and avoiding the call is more
// efficient here at this point
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these extra checks only avoid a function call, since the functions themselves already return early if the vector is empty, the benefit here seems minimal (just call-overhead savings). So I am not sure if you mean something additional by inefficiency of those calls?
Considering that, are you sure this TODO comments are actually needed? It feels like these TODOs might never be realistically addressed unless you have some ideas how to do it. If so, probably they should be just removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We generally have way too many function calls to do anything in the runtime. Eliminating unnecessary ones seems like a good goal. Calls into UR especially are not inlined due to the dispatching into plugins and all that, so avoiding unnecessary calls into UR definitely seems like a win.

@againull againull merged commit b762423 into intel:sycl Sep 25, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants