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

add clSetContextDestructorCallback #346

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

bashbaug
Copy link
Contributor

@bashbaug bashbaug commented Jul 8, 2020

Fixes #187.

This PR adds a new API to register a callback for a context that is called when the context is destroyed.

Copy link
Contributor

@kpet kpet left a comment

Choose a reason for hiding this comment

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

LGTM with one minor comment. This also begs the (separate) question: do we want to provide an alias to clSetProgramReleaseCallback for consistency? I'm thinking of clSetProgramDestructorCallback.

api/opencl_platform_layer.asciidoc Show resolved Hide resolved
@bashbaug
Copy link
Contributor Author

bashbaug commented Jul 8, 2020

This also begs the (separate) question: do we want to provide an alias to clSetProgramReleaseCallback for consistency? I'm thinking of clSetProgramDestructorCallback.

Since we don't support program scope constructors and destructors in OpenCL 3.0, I think a better thing to do is to deprecate clSetProgramReleaseCallback and add back clSetProgramDestructorCallback if and only if we add functionality that requires it.

@kpet
Copy link
Contributor

kpet commented Jul 9, 2020

Since we don't support program scope constructors and destructors in OpenCL 3.0,

We should clarify this in the spec BTW (Appendix H is pretty clear but the normative section just says "Missing before 2.2" which implies it's supported in 3.0). I've created #347.

I think a better thing to do is to deprecate clSetProgramReleaseCallback and add back clSetProgramDestructorCallback if and only if we add functionality that requires it.

Yes, I was thinking of the end state with the feature added back in, not specifically of 3.0 but I think it makes sense to deprecate in 3.0. I've created #348.

Copy link
Contributor

@alycm alycm left a comment

Choose a reason for hiding this comment

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

LGTM (the change to man/toctail made me notice other issues, filed that as #350 as nothing to do with this PR).

@keryell
Copy link
Member

keryell commented Jul 15, 2020

Since it is similar to some discussion we had in SYCL https://gitlab.khronos.org/sycl/Specification/-/issues/289 I wonder how this feature can be implemented safely.
For example what happens if the OpenCL runtime decide to destruct the context at exit, after main() exits?

@bashbaug
Copy link
Contributor Author

Interesting discussion, thanks for passing this along. FWIW, I don't think the SYCL proposal (link) is exactly the same as this feature because the proposed SYCL deleter gets called before the SYCL context is deleted, whereas this callback gets called after the OpenCL context is deleted, but there's definitely a fair amount of overlap.

For example what happens if the OpenCL runtime decide to destruct the context at exit, after main() exits?

I think the simplest answer is that this behavior won't be conformant. I'm hand-waving a little, but I imagine we would have a conformance test that ensures the context destructor callback is properly called, similar to this conformance test for clSetMemObjectDestructorCallback:

https://github.com/KhronosGroup/OpenCL-CTS/blob/master/test_conformance/api/test_mem_objects.cpp

Does that sound reasonable?

@bashbaug
Copy link
Contributor Author

Merging as discussed on the July 16th teleconference.

@bashbaug bashbaug merged commit 193879a into KhronosGroup:master Jul 16, 2020
@keryell
Copy link
Member

keryell commented Jul 16, 2020

Yes, there is a difference between when a context is deletable and when it has been actually discarded.
I am curious about the use case.
For example you create a static global C++ object that set up some OpenCL context in the constructor.
Probably it might be too late to be useful if the context destruction call-back is called on this static object destruction...

This pull request was closed.
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.

Need for clSetContextReleaseCallback?
5 participants