oneDPL icon indicating copy to clipboard operation
oneDPL copied to clipboard

Move `sycl::event` and `__result_and_scratch_storage` into `__future`

Open SergeyKopienko opened this issue 1 year ago • 7 comments

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;

SergeyKopienko avatar Aug 12 '24 10:08 SergeyKopienko

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

danhoeflinger avatar Aug 12 '24 12:08 danhoeflinger

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

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.

julianmi avatar Aug 12 '24 13:08 julianmi

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

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.

I like this variant! Due we pass only event id + 0/1param into the __future constructor now...

SergeyKopienko avatar Aug 12 '24 13:08 SergeyKopienko

@danhoeflinger, @reble I wrote new issue about our __future implementation: https://github.com/oneapi-src/oneDPL/issues/1776

SergeyKopienko avatar Aug 12 '24 15:08 SergeyKopienko

@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.

SergeyKopienko avatar Aug 12 '24 16:08 SergeyKopienko

We made decision together with @akukanov and @danhoeflinger to postpone this PR. So I clear milestone state and change it's state to Draft.

SergeyKopienko avatar Aug 14 '24 12:08 SergeyKopienko

@reble please take a look to this draft PR and take anything if it's useful for you.

SergeyKopienko avatar Aug 14 '24 12:08 SergeyKopienko