Skip to content

Conversation

reble
Copy link
Contributor

@reble reble commented Sep 16, 2025

There are two issues with the current implementation:

  • Creating a placeholder queue is adding overhead at finalize() to common path.
  • Queue properties are set to default when no queue can be reused at Graph finalize().

@reble reble changed the title [SYCL][Graph] Remove placeholder queue [SYCL][Graph] Reuse recording queue when available Oct 2, 2025
@reble reble changed the title [SYCL][Graph] Reuse recording queue when available [SYCL][Graph] Reuse recording queue when available for finalize Oct 2, 2025
@reble reble marked this pull request as ready for review October 6, 2025 16:07
@reble reble requested review from a team as code owners October 6, 2025 16:07
node_impl &add(std::shared_ptr<dynamic_command_group_impl> &DynCGImpl,
nodes_range Deps);

std::weak_ptr<sycl::detail::queue_impl> getQueue() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should return std::shared_ptr instead (or maybe a reference to weak). Creating a copy of the weak_ptr requies atomics and isn't much faster than creating a std::shared_ptr (if at all?, AFAIK). As such, creating a copy here followed by the lock/dtor at the use side is like triple atomics vs a single one when returning a shared pointer.

I might be totally wrong here, but that's my best understanding of how those things work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch and thanks for your feedback!
Let's use shared_ptr since it works better with the common case.
I don't think reference to weak is easily applicable, because we don't necessarily have a weak_ptr to return.
Even though I like the idea.

@reble reble force-pushed the pablo/improve-finalize branch from af7807d to 7cfd0dd Compare October 6, 2025 19:28
@reble reble temporarily deployed to WindowsCILock October 6, 2025 19:28 — with GitHub Actions Inactive
Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

github-actions bot commented Oct 6, 2025

@intel/llvm-gatekeepers please consider merging

@aelovikov-intel aelovikov-intel merged commit 01e439b into intel:sycl Oct 6, 2025
29 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