Skip to content

Remove the usages of __future on internal layers #2261

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

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented May 14, 2025

In this PR we remove the usage of __future class on internal layers of oneDPL code.

Instead of them now we using:

  • std::tuple<sycl::event>;
  • std::tuple<sycl::event, __result_and_scratch_storage>;
  • std::tuple<sycl::event, __result_and_scratch_storage_base_ptr>.
    These types are enough for implementation of required functional.

Now we create __future instances only in experimental async algorithm implementations.

Also in this PR we introduced new type __result_and_scratch_storage_base_ptr and declare it as

using __result_and_scratch_storage_base_ptr = std::unique_ptr<__result_and_scratch_storage_base>;

So now we are using __result_and_scratch_storage_base_ptr type instead of std::shared_ptr<__result_and_scratch_storage_base> in all code.

@SergeyKopienko
Copy link
Contributor Author

@danhoeflinger, I think you may continue thinks about re-design of __future class and so on.
On internal layers this class doesn't needed anymore.

@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/partially_replace_future branch 9 times, most recently from e876ce8 to fe1a183 Compare May 14, 2025 14:04
@SergeyKopienko SergeyKopienko marked this pull request as ready for review May 14, 2025 14:30
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/partially_replace_future branch from fe1a183 to 865f81d Compare May 14, 2025 16:32
@MikeDvorskiy
Copy link
Contributor

MikeDvorskiy commented Jun 3, 2025

First, I have a couple of questions:

  1. Which issue does PR solve?
  2. This is a significant architectural change. Was there a design review for a wide audience?

@SergeyKopienko
Copy link
Contributor Author

First, I have a couple of questions:

  1. Which issue does PR solve?
  2. This is a significant architectural change. Was there a design review for a wider audience?
  1. Which issue does PR solve?
    __future is something that make sense only for async experimental implementations. On internal layers it's not required, I believe.
  1. This is a significant architectural change. Was there a design review for a wider audience?
    Yes, it's planned.

@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/partially_replace_future branch from d42f486 to 6a1f61a Compare June 10, 2025 08:39
@SergeyKopienko SergeyKopienko requested a review from Copilot June 12, 2025 09:49
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the internal usage of the __future type by replacing it with tuple-based returns or direct event returns throughout the oneDPL hetero backend. The changes update various algorithm implementations and backend utilities so that asynchronous kernel submissions now return a std::tuple or sycl::event instead of a __future.

  • Removed __future wrapping and replaced with std::tuple<sycl::event, storage> patterns.
  • Updated return types and related documentation/comments in several hetero backend files.
  • Switched shared_ptr types to the new __result_and_scratch_storage_base_ptr for memory lifetime management.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.

File Description
include/oneapi/dpl/pstl/hetero/numeric_ranges_impl_hetero.h Updated __parallel_transform_reduce call to return a tuple and use __wait_and_get_value.
include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl_utils.h Removed internal __future methods and updated wait implementations.
include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl_merge_sort.h Changed shared_ptr usage to __result_and_scratch_storage_base_ptr.
... Other related files updated to use tuple returns or direct sycl::event.
Comments suppressed due to low confidence (3)

include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl_utils.h:736

  • Update the inline documentation for __wait_and_get_value to reflect that it is now used in tandem with the new __result_and_scratch_storage_base_ptr based return pattern instead of __future.
std::pair<std::size_t, std::size_t> __wait_and_get_value(const std::shared_ptr<__result_and_scratch_storage_base>& __p_storage)

include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl_merge_sort.h:591

  • Ensure that the naming convention for the new type __result_and_scratch_storage_base_ptr is applied consistently throughout the file after replacing shared_ptr, to avoid confusion in the API.
std::shared_ptr<__result_and_scratch_storage_base> __p_result_and_scratch_storage_base;

include/oneapi/dpl/pstl/hetero/numeric_ranges_impl_hetero.h:60

  • Confirm that using __storage.__wait_and_get_value(__event) in place of the previous get() call maintains the expected asynchronous behavior without introducing unintended blocking, and that it integrates seamlessly with the tuple-based return conventions.
return __storage.__wait_and_get_value(__event);

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

Successfully merging this pull request may close these issues.

2 participants