[SYCL][NFC] Cache the sycl::device instance in queue_impl #20764
Conversation
slawekptak
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
slawekptak
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
slawekptak
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
slawekptak
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@aelovikov-intel @lbushi25 Please see, if this kind of change could solve the shared_ptr unnecessary copies (for the device in this case). Thanks. |
| *sycl::detail::getSyclObjImpl(GraphImpl->getDevice()), | ||
| *sycl::detail::getSyclObjImpl(Context), sycl::async_handler{}, | ||
| sycl::property_list{}); | ||
| GraphImpl->getDevice(), *sycl::detail::getSyclObjImpl(Context), |
There was a problem hiding this comment.
Do we have a guarantee that graph will outlive queue?
| std::shared_ptr<queue_impl> InteropQueuePtr = queue_impl::create( | ||
| Dev, *InteropCtxPtr, async_handler{}, property_list{}); | ||
| std::shared_ptr<queue_impl> InteropQueuePtr = | ||
| queue_impl::create(createSyclObjFromImpl<device>(Dev), *InteropCtxPtr, |
There was a problem hiding this comment.
Definitely use-after free here, temporary device will die before the next statement and queue will have a stale reference. I'm also surprised it's compilable. How do you bind an rvalue device object to an lvalue reference argument?
There was a problem hiding this comment.
Yeah, I guess this compiles because the queue_impl constructor takes a const reference. It then makes a copy of the object, so it should work, but probably the constructor should take it by value - that should be clean, right?
| /// \param AsyncHandler is a SYCL asynchronous exception handler. | ||
| /// \param PropList is a list of properties to use for queue construction. | ||
| queue_impl(device_impl &Device, const async_handler &AsyncHandler, | ||
| queue_impl(const device &Device, const async_handler &AsyncHandler, |
There was a problem hiding this comment.
It feels quite suspicious to have an implementation-layer object take as a constructor argument a public API object. Can we not, for example, directly store the device object by adding it as a field to the queue class and modifying the queue constructors to retain the device object? If that is feasible, I would strongly prefer it instead.
|
I think a better change would be to make platform/device use raw pointers instead of I'm starting to work on that refactoring. |
Ok, let's do this then. Please feel free to include me in the review of that refactoring PR. |
3 participants
No description provided.