Move `sycl::event` and `__result_and_scratch_storage` into `__future`
In this PR we move the instances of sycl::event and __result_and_scratch_storage into __future constructor to avoid extra operations with counters inside std::shared_ptr objects which are the members of __result_and_scratch_storage (and probably in sycl::event implementation too).
Also we avoid copy operation for _ExecutionPolicy instance inside __result_and_scratch_storage too.
Please take a look to https://stackoverflow.com/questions/41871115/why-would-i-stdmove-an-stdshared-ptr for details.
Additionally, in this PR we fix issue https://github.com/oneapi-src/oneDPL/issues/1776 : we remove ability to create __future instance by copy and copy-assignment:
using FutureType = __future<_Event, _Args...>;
// ....
__future(const FutureType&) = delete;
__future(FutureType&&) = default;
FutureType&
operator=(const FutureType&) = delete;
FutureType&
operator=(FutureType&&) = default;
For this to stop the copy from happening, changes are required for __future as well I believe. I think @julianmi may be working on this already, we have discussed it a little.
Here is a godbolt that shows that even with this move, it still ends up with a copy when creating the tuple, unless we forward it in. The downside to such a change is it requires explicit specification of the __future type, rather than allowing it to use deduction. Julian has been looking at a way to at least allow the user to skip specifying the event type, but I don't think we can avoid requiring users to define the future value types. In the end, this may not be a bad thing, as it requires users of __future to be more deliberate with their types.
https://godbolt.org/z/vEWz6svMT
For this to stop the copy from happening, changes are required for
__futureas well I believe. I think @julianmi may be working on this already, we have discussed it a little.Here is a godbolt that shows that even with this move, it still ends up with a copy when creating the tuple, unless we forward it in. The downside to such a change is it requires explicit specification of the
__futuretype, rather than allowing it to use deduction. Julian has been looking at a way to at least allow the user to skip specifying the event type, but I don't think we can avoid requiring users to define the future value types. In the end, this may not be a bad thing, as it requires users of__futureto be more deliberate with their types.https://godbolt.org/z/vEWz6svMT
That's right. I've also included @reble in the discussion since he designed the original __future class. Another alternative would be to remove the tuple and specialize for our current use cases (event and/or one result container). However, this would be less generic. I would propose to close this PR and have an offline design discussion.
For this to stop the copy from happening, changes are required for
__futureas well I believe. I think @julianmi may be working on this already, we have discussed it a little. Here is a godbolt that shows that even with this move, it still ends up with a copy when creating the tuple, unless we forward it in. The downside to such a change is it requires explicit specification of the__futuretype, rather than allowing it to use deduction. Julian has been looking at a way to at least allow the user to skip specifying the event type, but I don't think we can avoid requiring users to define the future value types. In the end, this may not be a bad thing, as it requires users of__futureto be more deliberate with their types. https://godbolt.org/z/vEWz6svMTThat's right. I've also included @reble in the discussion since he designed the original
__futureclass. Another alternative would be to remove the tuple and specialize for our current use cases (event and/or one result container). However, this would be less generic. I would propose to close this PR and have an offline design discussion.
I like this variant!
Due we pass only event id + 0/1param into the __future constructor now...
@danhoeflinger, @reble I wrote new issue about our __future implementation: https://github.com/oneapi-src/oneDPL/issues/1776
@danhoeflinger , please take a look to this PR again.
Probably this __future implementations is ok for us?
Also I fixed issue https://github.com/oneapi-src/oneDPL/issues/1776 here.
We made decision together with @akukanov and @danhoeflinger to postpone this PR. So I clear milestone state and change it's state to Draft.
@reble please take a look to this draft PR and take anything if it's useful for you.