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 in_place_scope_fifo #855

Merged
merged 1 commit into from
May 15, 2021
Merged

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented May 4, 2021

This matches the recent in_place_scope, but using ScopeFifo. The
scope constructors have also been refactored to avoid duplication.

This matches the recent `in_place_scope`, but using `ScopeFifo`. The
scope constructors have also been refactored to avoid duplication.
@cuviper
Copy link
Member Author

cuviper commented May 4, 2021

cc @rocallahan

///
/// Unsafe because it must be executed on a worker thread.
unsafe fn complete<FUNC, R>(&self, owner: Option<&WorkerThread>, func: FUNC) -> R
fn complete<FUNC, R>(&self, owner: Option<&WorkerThread>, func: FUNC) -> R
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not unsafe? I guess it never had to be...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, at least the comment "because it must be executed on a worker thread" is optional after #844. I didn't really change that functionality in this PR, just cleaning up.

I suppose there's a correctness issue if Some(owner) doesn't match the latch, but we do have debug_assert_eq for that, and it's not really a safety concern. Worst case is that we can't wake up the right thread. We control all the callers in this same module though, and they do trivially match from new to complete.

(We can't put the &WorkerThread directly in the scope or latch because it's not Sync.)

},
None => ScopeLatch::Blocking {
latch: CountLockLatch::new(),
},
Copy link
Member

Choose a reason for hiding this comment

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

+1, nice

@nikomatsakis
Copy link
Member

r+ from my side :) @cuviper were you ready to merge?

@cuviper
Copy link
Member Author

cuviper commented May 15, 2021

Sure!

bors r=nikomatsakis

@bors bors bot merged commit 3c7489e into rayon-rs:master May 15, 2021
@cuviper cuviper deleted the in_place_scope_fifo branch February 25, 2023 17:58
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