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

Document implicit yield in install() per #1105 #1107

Merged
merged 2 commits into from Dec 13, 2023

Conversation

benkay86
Copy link
Contributor

Add documentation to rayon::ThreadPool::install() warning about implicit yield behavior, as discussed in #1105.

Comment on lines 85 to 87
/// `install()` implicitly calls [`ThreadPool::yield_now()`],
/// potentially scheduling another task to run on the current thread. For
/// example:
Copy link
Member

Choose a reason for hiding this comment

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

I would like this to clarify the context a bit more, something like:

Suggested change
/// `install()` implicitly calls [`ThreadPool::yield_now()`],
/// potentially scheduling another task to run on the current thread. For
/// example:
/// If the current thread is part of a different thread pool, it will try to
/// keep busy while the `op` completes in its target pool, similar to
/// calling [`ThreadPool::yield_now()`] in a loop. Therefore, it may
/// potentially schedule other tasks to run on the current thread in the
/// meantime. For example:

Does that make sense to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, changed to your phrasing via force push.

@cuviper
Copy link
Member

cuviper commented Dec 13, 2023

Thanks!

/// fn main() {
/// rayon::ThreadPoolBuilder::new().num_threads(1).build_global().unwrap();
/// let pool = rayon::ThreadPoolBuilder::default().build().unwrap();
/// (0..3).into_par_iter().for_each(|_| {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, we can't use APIs from rayon proper while testing rayon-core. Maybe you can make the example with join instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Foiled! Rewritten to use join() as requested.

@cuviper cuviper added this pull request to the merge queue Dec 13, 2023
Merged via the queue into rayon-rs:master with commit 40e2099 Dec 13, 2023
4 checks passed
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.

None yet

2 participants