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.

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

Clarify behavior of adapter handles when being reinitialized #781

Closed
callumfare opened this issue Aug 4, 2023 · 5 comments
Closed

Clarify behavior of adapter handles when being reinitialized #781

callumfare opened this issue Aug 4, 2023 · 5 comments
Labels
specification Changes or additions to the specification

Comments

@callumfare
Copy link
Contributor

A potential problem has come up where the UR conformance tests release the adapter(s), dropping their refcount to 0, and then reinitialize them with urAdapterGet in the next test.

The spec is not clear whether this is allowed or not, but it currently works for the existing implementations.

I'm in favor of allowing this behavior as it supports use cases where there are multiple language runtimes using UR, but not simultaneously, so when one finishes and unloads the adapters the other can still safely initialize UR and use it. I'm not sure if there are any cases where we would have global adapter state that's not safe to reinitialize after being released. The entry points should already be thread safe.

A related issue is that the spec descriptions for urInit and urTearDown are out-of-date as they still refer to adapter state. In reality these are only now useful for loader state, so this needs updated. I don't think this really needs discussion so I can make that change shortly. We could also rename them urLoaderInit and urLoaderTearDown if that would help clarify their purpose.

@callumfare callumfare added the specification Changes or additions to the specification label Aug 4, 2023
@pbalcer
Copy link
Contributor

pbalcer commented Aug 4, 2023

Sounds good to me. Not being able to reinitialize adapter state would be surprising.

If we were to rename init/teardown APIs, should they become loader-only? I think they should. We could also make them optional.

@callumfare
Copy link
Contributor Author

Making them loader-only sounds good to me. What would it mean if they were optional? That the adapter libraries could be used directly without the loader, or that it would be possible to use the loader without initializing it?

@pbalcer
Copy link
Contributor

pbalcer commented Aug 7, 2023

I meant that we could just initialize loader on first use. There's already an indirection on every ur entry point, so we could do this with no performance cost. But I'm not sure if slightly simplifying this API is worth the complication.

@callumfare
Copy link
Contributor Author

That makes sense, I've started making the changes without making it optional for now to keep things simpler.

I've run into a problem with the validation layer - if urTearDown (now urLoaderTearDown) is loader-only, the call won't go through the validation layer and we can't use it to log the leaked references at the end of execution. An alternative would be to track the references per adapter (I'm not actually sure if that's possible with the way the layers are set up right now), and then print the leaks when the final reference to the adapter is released. However because we don't have a urAdapterCreate function it doesn't really fit into the existing reference tracking, even though urAdapterGet does actually reference the increment count. Incidentally I'm not sure that reference tracking of devices works right now for the same reason, urDeviceRetain and urDeviceRelease control the reference count but urDeviceGet never actually tracks the device handle.

@callumfare
Copy link
Contributor Author

Resolved by #793

The layer now reports leaked stats when all adapters are unloaded, or when the layer is destroyed, so the problems referenced in the previous comment have been addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
specification Changes or additions to the specification
Projects
None yet
Development

No branches or pull requests

2 participants