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

Issue with using __future as an event with keep-alive #1973

Open
danhoeflinger opened this issue Dec 18, 2024 · 1 comment
Open

Issue with using __future as an event with keep-alive #1973

danhoeflinger opened this issue Dec 18, 2024 · 1 comment
Assignees
Labels

Comments

@danhoeflinger
Copy link
Contributor

oneDPL currently uses the internal __future class as a return from sycl implementations, which has the side-effect of keeping alive any temporary data included in the __future until it leaves scope. It is used in some cases where no value is returned from the algorithm, and only for this side effect of keeping alive data. This works as written, but only because the future is only waited upon, and get() is not called to attempt to get the return value. This creates a maintenance problem and really is an abuse of __future, as subsequent changes may misunderstand this hidden requirement and result in a bug.

This is currently done in transform_scan APIs with the "reduce_then_scan" implementation (here). It also seems to be relevant for the new optimization of merge. I would recommend for that PR to handle this similarly to transform_scan, rather than employing a separate approach for now. When we move forward with the below proposal or some other solution, we can adjust both cases to use the new solution.

oneDPL should add some internal type __event_with_keepalive which provides the same functionality from __future as we need for these cases without the danger of a get() routine in its interface. Ideally, this new type can share implementation details with __future to prevent redundancy, and also, should be interoperable with __future, to allow a future to based upon an __event_with_keepalive as its event. This will allow the writer of the code to separate values for which a result is expected and values merely included to keep them alive when returning from an internal asynchronous call.

Part of the resolution of this issue should also include a search through current usage of __future to find any which should be converted to __event_with_keepalive.

@danhoeflinger
Copy link
Contributor Author

I'll further note a compounding issue that for both of these cases linked above, we have a runtime decision to dispatch to 1 of 2 kernels, where in each case only 1 kernel needs temporary storage where the other does not. For both cases, we need some "dummy" temporary storage for the kernels which do not need temporary storage so we can always return the same type object from both branches of our runtime-selected kernel call.

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

No branches or pull requests

3 participants