Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Consider properly reference counting and managing loader objects #1784

Open
aarongreig opened this issue Jun 25, 2024 · 1 comment · May be fixed by #2048
Open

Consider properly reference counting and managing loader objects #1784

aarongreig opened this issue Jun 25, 2024 · 1 comment · May be fixed by #2048
Assignees
@RossBrunton

Comments

@aarongreig
Copy link
Contributor

This PR adding [release] tags to entry points that decrement handle reference counts revived a seemingly long dead mechanism in loader code: generating calls to the object factory's release method, which removes dead handles from the loader/platform handle map.

We inherited this mechanism from level zero, which doesn't have Retain entry points or any other way to increment a handle's reference count. As a result there isn't a reference counting mechanism in the loader to make sure we're only erasing handles from the map when they're actually invalid, the first call to Release a given handle would always delete it from the map if we allowed this code to generate as it was originally designed.

Introducing a reference count to the loader objects does mean a slight additional overhead, and until now we haven't noticed the absence of this mechanism which only really serves to keep the size of the factory object maps down. This could also have weird consequences for objects created from native handles, it's a change we'll need to carefully consider and design properly if the benefits seem worth it.

For now I've disabled generating these release calls in #1720, effectively making it a NFC
@pbalcer
Copy link
Contributor

pbalcer commented Jun 25, 2024 •
edited
Loading

My two cents: we should should reconsider using the loader handler wrappers altogether. They are massive PITA. Sometime ago I even added a task for myself to look into better approaches, but haven't had the chance to work on that yet. But I did think on it, and came to the conclusion that we should just require adapters to have a pointer to the DDI in the first 8 bytes of any handle (except for native ones). Which is I believe what OpenCL does.
Yes, it would make it a tiny bit harder to do inheritance (objects with vptr's would need wrappers), but that's a small price to pay for eliminating the back-and-forth translation of handles.

@RossBrunton RossBrunton self-assigned this Sep 3, 2024
RossBrunton added a commit to RossBrunton/unified-runtime that referenced this issue Sep 4, 2024
@RossBrunton
Use reference counting on factories
cc693d5 
Previously the factories used by ur_ldrddi (used when there are multiple
backends) would add newly created objects to a map, but never release
them. This patch adds reference counting semantics to the allocation,
retention and release methods.

No tests were added; this should be covered by tests for urThingRetain.

Closes: oneapi-src#1784 .
@RossBrunton RossBrunton linked a pull request Sep 4, 2024 that will close this issue
Open
RossBrunton added a commit to RossBrunton/unified-runtime that referenced this issue Sep 5, 2024
@RossBrunton
Use reference counting on factories
6898550 
Previously the factories used by ur_ldrddi (used when there are multiple
backends) would add newly created objects to a map, but never release
them. This patch adds reference counting semantics to the allocation,
retention and release methods.

No tests were added; this should be covered by tests for urThingRetain.

Closes: oneapi-src#1784 .
RossBrunton added a commit to RossBrunton/unified-runtime that referenced this issue Sep 6, 2024
@RossBrunton
Use reference counting on factories
e00cb3e 
Previously the factories used by ur_ldrddi (used when there are multiple
backends) would add newly created objects to a map, but never release
them. This patch adds reference counting semantics to the allocation,
retention and release methods.

No tests were added; this should be covered by tests for urThingRetain.

Closes: oneapi-src#1784 .
RossBrunton added a commit to RossBrunton/unified-runtime that referenced this issue Sep 9, 2024
@RossBrunton
Use reference counting on factories
2875a5e 
Previously the factories used by ur_ldrddi (used when there are multiple
backends) would add newly created objects to a map, but never release
them. This patch adds reference counting semantics to the allocation,
retention and release methods.

No tests were added; this should be covered by tests for urThingRetain.

Closes: oneapi-src#1784 .
RossBrunton added a commit to RossBrunton/unified-runtime that referenced this issue Sep 10, 2024
@RossBrunton
Use reference counting on factories
3a6ed49 
Previously the factories used by ur_ldrddi (used when there are multiple
backends) would add newly created objects to a map, but never release
them. This patch adds reference counting semantics to the allocation,
retention and release methods.

No tests were added; this should be covered by tests for urThingRetain.

Closes: oneapi-src#1784 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RossBrunton RossBrunton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use reference counting on factories RossBrunton/unified-runtime
3 participants
@RossBrunton @pbalcer @aarongreig