Skip to content

Conversation

@Alexandr-Konovalov
Copy link
Contributor

@Alexandr-Konovalov Alexandr-Konovalov commented Mar 5, 2025

There are two set of changes:

  1. On hot path handler_impl is allocated on stack.
  2. handler keeps reference to queue shared_ptr, not shared_ptr.

@vinser52
Copy link
Contributor

vinser52 commented May 5, 2025

@againull Could you please review?

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Fix grammar in the PR title:
"[SYCL] Allocate sycl::handler in stack when possible" ->
"[SYCL] Allocate sycl::handler on the stack when possible"

@Alexandr-Konovalov Alexandr-Konovalov changed the title [SYCL] Allocate sycl::handler in stack when possible [SYCL] Allocate sycl::handler on the stack when possible May 6, 2025
Co-authored-by: Alexey Bader <[email protected]>
Co-authored-by: Alexey Bader <[email protected]>
Co-authored-by: Alexey Bader <[email protected]>
@Alexandr-Konovalov
Copy link
Contributor Author

@intel/llvm-reviewers-runtime , Colleagues, I see ThreadSanitizer/check_usm.cpp failed for other PRs as well, and there are 2 approvals. What are possible next steps?

@vinser52
Copy link
Contributor

vinser52 commented May 7, 2025

@intel/llvm-reviewers-runtime , Colleagues, I see ThreadSanitizer/check_usm.cpp failed for other PRs as well, and there are 2 approvals. What are possible next steps?

I think the issue is unrelated to this PR, see #17486

@sergey-semenov sergey-semenov merged commit 3a5ee6e into intel:sycl May 7, 2025
22 of 23 checks passed

handler::handler(const std::shared_ptr<detail::queue_impl> &Queue,
bool CallerNeedsEvent)
: MImplOwner(std::make_shared<detail::handler_impl>(Queue.get(),
Copy link
Contributor

@aelovikov-intel aelovikov-intel May 30, 2025

Choose a reason for hiding this comment

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

Why Queue.get() here instead of nullptr? handler_impl ctor expects secondary queue here, not primary one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, opened PR #18752 with the fix.

@Alexandr-Konovalov Alexandr-Konovalov deleted the Alexandr-Konovalov/handler-in-stack branch September 15, 2025 08:29
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.

7 participants