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

[WIP] P2300 bulk implementation to libunifex #354

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[WIP] P2300 bulk implementation to libunifex #354

wants to merge 1 commit into from

Conversation

RishabhRD
Copy link
Contributor

This PR intends to add P2300 std::execution::bulk implementation to libunifex under name unifex::bulk.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 30, 2021
@ispeters
Copy link
Contributor

The PR title says WIP, which I read as "work in progress", but there's no activity since September 30; do you intend to iterate, or are you waiting for a review?

@RishabhRD
Copy link
Contributor Author

@ispeters Apologies, I was unable to pay attention to the PR from a long time because I am going through my undergraduate job interviews, so I got busy in preparation and interview stuffs. It would take 10-12 more days for interviews to be over. I would start working on this PR after that.

Currently the PR just fulfills the spec as described in P2300, however unifex::bulk_transform and other related modules are doing some extra work that I am not sure of and I need to read that. Based on that I would progress on the current PR once I start working.

If someone is doing the same work feel free to close this PR in favor of that as I still need 10-12 days to start working and project progress is way more important.

Thanks

@ispeters
Copy link
Contributor

Oh, goodness, @RishabhRD, no need to apologize, I was just checking in. If you intend to iterate when you have time, we'll wait for you. Thanks for the update, and good luck with those interviews!

@RishabhRD
Copy link
Contributor Author

@ispeters I need to discuss some of implementation details from unifex::bulk_transform and unifex::bulk_schedule.

Both of the modules use unifex::set_next for every index in shape space. This set_next is not proposed by P2300. Currently my implementation does the things prescribed in P2300. Should I add set_next thing to my implementation.

Behavioral difference between set_next implementation and P2300 proposal is:

  • bulk_transform can accept sender created by bulk_schedule and accepts an invocable. bulk_schedule calls unifex::set_next for every index in shape space. bulk_transform's internal receiver implements set_next that invoke the given invocable with index passed by bulk_schedule to unifex::set_next and then it in turn calls unifex::set_next with the result of invocation of given function.
    Function Signature of set next: unifex::set_next(Receiver, index)

  • P2300 proposes that execution::bulk accepts a sender, shape and a invocable. When set_value would be called, set_value implementation of bulk's internal receiver should invoke given invocable for all index in given shape space along with the value passed to set_value function and then call execution::set_value with those value passed.

In the above implementation the invocable has nothing to do with values passed in set_value. So, they are quite different. So, should I choose set_next direction?

Comment on lines +51 to +53
for (Shape i{0}; i < shape_; i++) {
func_(i, values...);
}
Copy link
Contributor Author

@RishabhRD RishabhRD Nov 22, 2021

Choose a reason for hiding this comment

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

Here in start of each iteration of loop, there is a chance where we can do cancellation stuff with calling unifex::set_done and not proceeding for further iteration. unifex::bulk_schedule also does this stuff in implementation.

for (Shape i{0}; i < shape_; i++) {
      if(stop_token.stop_requested()) {
             unifex::set_done(std::move(receiver_));
             return;
      }
}

This looks to be legitimate as the shape_ can be big and iteration can be time consuming and this can be a good stop point. Is it fine for everyone?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's probably correct. For a small enough granularity of iterations though it would be quite expensive.

@ispeters
Copy link
Contributor

ispeters commented Feb 9, 2022

@LeeHowes, I think you're the expert on the bulk_ methods; do you have opinions on @RishabhRD's questions?

@LeeHowes
Copy link
Contributor

LeeHowes commented Feb 9, 2022

Yes, I think matching the p2300 design makes most sense at this point, even with all of its flaws. P2300 looks like it is removing the default looping implementation of bulk, so really the appropriate thing to do would be to customise on the scheduler so that there is a bulk implementation for each of the built in execution contexts.

P2300 uses the clunky method of propagating a scheduler with the senders, I don't think that's implemented in libunifex, though. So this would probably mean also implementing the get_completion_scheduler functionality as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants