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

Add rmm::exec_policy_nosync #1009

Merged
merged 2 commits into from
Apr 26, 2022

Conversation

fkallen
Copy link
Contributor

@fkallen fkallen commented Mar 26, 2022

Add rmm:exec_policy_nosync.

For Thrust 1.16 and newer, this policy is based on execute_on_stream_nosync_base instead of execute_on_stream_base to allow Thrust algorithms to skip synchronizations.

For older Thrust versions, this is an alias to rmm::exec_policy

@fkallen fkallen requested a review from a team as a code owner March 26, 2022 11:51
@fkallen fkallen requested review from rongou and cwharris March 26, 2022 11:51
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

1 similar comment
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@github-actions github-actions bot added the cpp Pertains to C++ code label Mar 26, 2022
@jrhemstad
Copy link
Contributor

jrhemstad commented Mar 28, 2022

tbh, I'd been thinking about just changing the rmm::exec_policy to use the nosync version.

Technically this would be a breaking change, but I'd be surprised if anything in RAPIDS was depending on the synchronous behavior of Thrust algorithms. I suspect most would appreciate the elimination of the extra syncs.

I'll see what @harrism thinks

@harrism
Copy link
Member

harrism commented Mar 29, 2022

but I'd be surprised if anything in RAPIDS was depending on the synchronous behavior of Thrust algorithms.

Really? I won't be surprised. Not saying I oppose it, just that we should do an investigation before flipping a switch like this.

@jrhemstad
Copy link
Contributor

Really? I won't be surprised.

What can I say, people always tell me I'm such an optimist ¯_(ツ)_/¯

To be clear, algorithms like thrust::reduce would still synchronize with the nosync policy because they have to.

The nosync policy controls if an algorithm like thrust::transform or thrust::for_each synchronizes or not. I'd hope we didn't have code relying on that behavior.

@harrism
Copy link
Member

harrism commented Mar 31, 2022

Just saying we should test all RAPIDS libraries before merging such a change (not just RMM tests). Also test Spark since they rely on PTDS.

@jrhemstad
Copy link
Contributor

Just saying we should test all RAPIDS libraries before merging such a change (not just RMM tests). Also test Spark since they rely on PTDS.

I think I've changed my mind. I don't think the hassle of having to verify every usage of rmm::exec_policy is worth it. It's easy enough to find/replace rmm::exec_policy with rmm::exec_policy_async in consumer libraries.

Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

Thanks @fkallen! Could you also add some tests?

@fkallen
Copy link
Contributor Author

fkallen commented Apr 3, 2022

Do you have a test in mind? I don't know how to test this other than taking a look at the profiler to check if there is a pool allocation and a missing sync.

@harrism harrism added feature request New feature or request non-breaking Non-breaking change labels Apr 4, 2022
@harrism harrism added this to PR-WIP in v22.06 Release via automation Apr 4, 2022
@jrhemstad
Copy link
Contributor

Do you have a test in mind? I don't know how to test this other than taking a look at the profiler to check if there is a pool allocation and a missing sync.

I was just thinking making sure that it works as a valid execution policy. So simply launch a Thrust algorithm with device data and make sure the result is sound.

I was going to say "just do what we do for our current tests", but apparently that's nothing!

@harrism
Copy link
Member

harrism commented Apr 5, 2022

ok to test

@harrism
Copy link
Member

harrism commented Apr 5, 2022

rerun tests

v22.06 Release automation moved this from PR-WIP to PR-Reviewer approved Apr 5, 2022
Copy link
Contributor

@cwharris cwharris left a comment

Choose a reason for hiding this comment

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

nosync isn't a term we use in rmm, or any rapids libraries as far as I know. What if we use async, which is common?

Nevermind, I see now that this is based on thrust policy.

include/rmm/exec_policy.hpp Show resolved Hide resolved
v22.06 Release automation moved this from PR-Reviewer approved to PR-Needs review Apr 8, 2022
v22.06 Release automation moved this from PR-Needs review to PR-Reviewer approved Apr 8, 2022
@harrism
Copy link
Member

harrism commented Apr 26, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 573351d into rapidsai:branch-22.06 Apr 26, 2022
v22.06 Release automation moved this from PR-Reviewer approved to Done Apr 26, 2022
@fkallen fkallen deleted the execpolicythrustnosync branch August 9, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp Pertains to C++ code feature request New feature or request non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants