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

Disable copy-constructor and copy-assignment operations for the __future class #1859

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented Sep 18, 2024

Issue: #1776

What we change in this PR:

  • remove copy constructor and copy assignment from the __future : to be closet for std::future implementation and to avoid multiple resource (`sycl::event') owners;
  • change format of __future constructors: in all use cases we able to move sycl::event into the constructor instead of it's copy;
  • change format of __future constructors: we pass std::tuple into constructor by const reference or as r-value;
  • we change the semantic of __make_future functions: we declare it as static and rewrite it to avoid extra data copy and use data move if possible.

@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/delete_copy_op_from_future branch 5 times, most recently from 4e27832 to 3fdf9f1 Compare September 19, 2024 13:41
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

I would resist making direct changes until we have had the chance to discuss this holistically in the context of the asynchronous API discussion.
It is a public (experimental) API, so we may want to wait and then change it only a single time, in a deliberate way (perhaps in the context of productization).

@SergeyKopienko
Copy link
Contributor Author

I would resist making direct changes until we have had the chance to discuss this holistically in the context of the asynchronous API discussion. It is a public (experimental) API, so we may want to wait and then change it only a single time, in a deliberate way (perhaps in the context of productization).

Unfortunately, not only (It is a public (experimental) API) : we widely using __future inside our code I would like to made it a little bit more optimal.

@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/delete_copy_op_from_future branch from 25f1ee1 to 770f606 Compare September 24, 2024 08:18
@danhoeflinger danhoeflinger requested a review from reble September 24, 2024 19:16
…delete copy constructor from oneapi::dpl::__par_backend_hetero::__future class

Signed-off-by: Sergey Kopienko <[email protected]>
…delete copy assignment from oneapi::dpl::__par_backend_hetero::__future class

Signed-off-by: Sergey Kopienko <[email protected]>
…move parameters in oneapi::dpl::__par_backend_hetero::__future class constructors

Signed-off-by: Sergey Kopienko <[email protected]>
…uture improvements : make code more compact

Signed-off-by: Sergey Kopienko <[email protected]>
…modify the set of __future constructors

Signed-off-by: Sergey Kopienko <[email protected]>
… special fix for __parallel_transform_reduce_work_group_kernel_submitter::operator()

Signed-off-by: Sergey Kopienko <[email protected]>
Signed-off-by: Sergey Kopienko <[email protected]>
…f auto"

This reverts commit 6be8e44.

Signed-off-by: Sergey Kopienko <[email protected]>

# Conflicts:
#	include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl_reduce.h
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/delete_copy_op_from_future branch from 770f606 to 3afc10c Compare September 25, 2024 08:26
@SergeyKopienko SergeyKopienko marked this pull request as draft December 20, 2024 12:20
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.

2 participants