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 full_blocking feature to thread pool #1175

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

Conversation

nhukc
Copy link

@nhukc nhukc commented Jun 18, 2024

This PR adds an option full_blocking to ThreadPoolBuilder that affects the behavior of nested thread pools.

When a job on a parent thread pool creates a job in a child thread pool and full_blocking is true, the parent job will block until the child job is completed. This is different from the default behavior where the parent thread is allowed to work on other jobs in the parent thread pool while the child job completes.

This behavior is useful for avoiding deadlocks caused by work-stealing and is helpful for instrumentation based profiling in multi-threaded settings.

See for more details:
#1174

@nhukc
Copy link
Author

nhukc commented Jun 18, 2024

Feel free to push directly to this PR.

The documentation could be improved by matching the language to the rest of the rayon docs.

I'm not 100% confident in the implementation of in_worker_cross_blocking. It works for my test, but I could easily be missing something in the details.

Copy link
Contributor

@JoeyBF JoeyBF left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! One small suggestion would be to make full_blocking take a bool as an argument, so it can be toggled on or off. This seems to be more in line with the other builder methods.

I'll add that I tested my code using this branch and this does solve my own issue #957, which is essentially a variation of #592 🎉

rayon-core/src/lib.rs Show resolved Hide resolved
tests/issue592.rs Show resolved Hide resolved
rayon-core/src/thread_pool/test.rs Show resolved Hide resolved
@nhukc
Copy link
Author

nhukc commented Jun 24, 2024

Thanks for taking a look at the PR!

The most similar function in the ThreadPoolBuilder is

pub fn use_current_thread(self) -> ThreadPoolBuilder<S>

Which sets a bool to true when called. I matched that function signature.

@JoeyBF
Copy link
Contributor

JoeyBF commented Jun 24, 2024

That's fair. I would argue that use_current_thread would also need a bool, but as far as this PR is concerned it's probably better to just be consistent.

@nhukc
Copy link
Author

nhukc commented Jun 26, 2024

@cuviper Are you interested in merging this? If not, can you close it?

@cuviper
Copy link
Member

cuviper commented Jun 26, 2024

I need some time to think about the implications of this change, to make sure there aren't any big foot-guns here. But you're also helping solve a foot-gun, so that's going to be a balancing act.

I might also bikeshed the name, but that's not a big deal.

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

3 participants