Skip to content

[SYCL] Less shared_ptr for platform_impl #18143

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

Merged
merged 1 commit into from
Apr 28, 2025

Conversation

aelovikov-intel
Copy link
Contributor

@aelovikov-intel aelovikov-intel commented Apr 22, 2025

GlobalHandler::MPlatformCache keeps (shared) ownership of platform_impl objects, so none of them could be destroyed until SYCL RT library shutdown/unload process. As such, using raw pointers/reference to platform_impl throughout the SYCL RT is totally fine and would avoid extra costs of std::shared_ptr. However, our unittests don't work the same way and lifetimes are actually managed by all the data member std::shared_ptrs (e.g., see CurrentDevice.cpp as the most obvious case), so simply switching all PlatformImplPtr to raw pointer/reference doesn't work right now. I think it will be possible with an ABI break if we'd remove all the shared pointers but the one in the GlobalHandler.

What I'm doing here instead is the following:

  • Try to keep lifetime management the same, i.e., all data member pointers are still "smart"
  • Inherit from std::enable_shared_from_this() so that we could pass raw references around and only actually create smart pointers when absolutely necessary.
  • Change internal APIs to operate more using plain references

This should make it much harder to write something like this:

  // BAD: Should have been `auto &`, extra atomic counter
  // increment/decrement without it.
  auto PlatformImpl = d.getPlatformImplPtr();

@uditagarwal97 uditagarwal97 requested a review from Copilot April 24, 2025 17:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the SYCL runtime to reduce unnecessary use of std::shared_ptr for platform_impl objects by replacing them with raw references or pointers in critical parts of the code, thereby reducing ownership overhead. The changes update several interfaces and call sites throughout the codebase.

  • Changed the platform interface in tests and source files (e.g. printers.cpp, platform.cpp) to use platform_impl references.
  • Updated device and program manager implementations to adjust method calls and data access to the new raw reference API.
  • Refactored platform_impl.hpp/.cpp and related files to expose getSharedPtrToSelf() and return references instead of smart pointers.

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

Show a summary per file
File Description
sycl/test/gdb/printers.cpp Updated comments to reflect the change from shared_ptr to references for platform_impl.
sycl/source/platform.cpp Modified platform_impl acquisition to return a shared pointer via getSharedPtrToSelf() and updated related call sites.
sycl/source/device.cpp Adjusted device_impl initialization to use the new platform_impl API with references.
sycl/source/detail/usm/usm_impl.cpp Replaced arrow operator calls with dot operator calls on platform_impl references.
sycl/source/detail/program_manager/program_manager.cpp Refactored access to platform_impl from pointer to reference in option handling and backend option retrieval.
sycl/source/detail/platform_impl.hpp/.cpp Changed several methods in platform_impl to return references instead of shared_ptr, including updates to helper methods and caching.
Other Files (global_handler.hpp/.cpp, device_impl.hpp/.cpp, context_impl.hpp/.cpp, backend files, etc.) Updated all affected call sites and interface definitions to conform with the new ownership model.

@againull againull merged commit 73535d9 into intel:sycl Apr 28, 2025
25 checks passed
@aelovikov-intel aelovikov-intel deleted the platform_impl branch April 28, 2025 22:45
aelovikov-intel added a commit that referenced this pull request Apr 30, 2025
…8251)

After that devices are never destroyed until the SYCL RT library
shutdown. In practice, that means that before the change a simple

```
int main() { sycl::device d; }
```

went into `platform` ctor, then queried all the platform's devices to
check that it has some, returned from ctor and those `sycl::device`s
created on stack were already destroyed. After that, when creating
user's `sycl::device d` we were re-creating device hierarchy for the
platform at SYCL level again (including some calls to `urDeviceGetInfo`
during `device_impl` creation).

After the changes, devices created when veryfing that platform isn't
empty are preserved inside the `platform_impl` object and this existing
SYCL devices hierarchy is used when creating user's device object.

A note on the implementation: `device_impl` has an
`std::shared_ptr<platform_impl>` inside so we can't rely on automatic
resource management just by the nature of `std::shared_ptr` everywhere
(and we haven't changed this aspect in
#18143). As such, we have to perform
some explicit resource release during shutdown procedure (or in
`~UrMock()` for unittests).
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Apr 30, 2025
After intel#18251 device are guaranteed to
be alive until SYCL RT library shutdown, so we don't have to pass
everything in `std::shared_ptr<device_impl>` and might use raw
pointers/references much more.

That said, constraints from
intel#18143 (mostly unittests linking
statically and lifetimes of static/thread-local objects following from
that) are still here and I'm addressing them the same way - not totally
changing the ownership model, using `std::enable_shared_from_this` and
keep creating shared pointers for member objects to keep the graph of
resource ownership intact.
aelovikov-intel added a commit that referenced this pull request May 1, 2025
After #18251 devices are guaranteed to
be alive until SYCL RT library shutdown, so we don't have to pass
everything in `std::shared_ptr<device_impl>` and might use raw
pointers/references much more.

That said, constraints from
#18143 (mostly unittests linking
statically and lifetimes of static/thread-local objects following from
that) are still here and I'm addressing them the same way - not totally
changing the ownership model, using `std::enable_shared_from_this` and
keep creating shared pointers for member objects to keep the graph of
resource ownership intact.
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.

2 participants