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

core: Introduce ThreadPoolBuilder::use_current_thread. #1063

Merged
merged 6 commits into from
Sep 20, 2023

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Jun 25, 2023

This generalizes the approach used by targets that don't support threading like
wasm, allowing the builder thread to be part of a new thread-pool.

This PR:

  • Builds on top of the PoC implementation from that issue.
  • Renames the API as per the comments there.
  • Adds a way to clean up the WorkerThread storage once the pool is dropped.
  • Documents and tests the APIs.

Feedback welcome. clean_up_use_current_thread is not a great name, but I
think it's descriptive, and maybe good enough given it's a rather niche API for
non-global pools?

@emilio
Copy link
Contributor Author

emilio commented Jun 25, 2023

r? @cuviper

@emilio
Copy link
Contributor Author

emilio commented Jun 25, 2023

Fixed the formatting of the tests, whoops.

@emilio
Copy link
Contributor Author

emilio commented Jul 11, 2023

@cuviper review ping? Modulo the naming issue discussed above (happy to tweak as needed) I think this should be in decent shape?

@emilio
Copy link
Contributor Author

emilio commented Jul 11, 2023

Also ideas to be able to test the error (deadlocking) case of calling clean_up_use_current_thread from the inner job would be great, but probably not a blocker.

@cuviper
Copy link
Member

cuviper commented Jul 11, 2023

Hi, sorry -- I'm going to be unavailable the rest of this week, but I'll try to get to this early next week.

@emilio
Copy link
Contributor Author

emilio commented Jul 11, 2023

No worries, I was just making sure this didn't fall through the cracks, thanks! (And enjoy your time off if that's the reason for your availability :)).

@emilio
Copy link
Contributor Author

emilio commented Aug 10, 2023

Gentle ping?

rayon-core/src/lib.rs Outdated Show resolved Hide resolved
rayon-core/src/lib.rs Outdated Show resolved Hide resolved
@emilio emilio force-pushed the use-current-thread branch 7 times, most recently from 327626a to 38c411b Compare August 23, 2023 19:26
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 29, 2023
…r=smaug,jnicol,supply-chain-reviewers

This applies rayon-rs/rayon#1063 to our
rayon-core. I'm hopeful it can be merged upstream soon, but meanwhile
this seems worth having on its ow.

Differential Revision: https://phabricator.services.mozilla.com/D186722
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Aug 29, 2023
…r=smaug,jnicol,supply-chain-reviewers

This applies rayon-rs/rayon#1063 to our
rayon-core. I'm hopeful it can be merged upstream soon, but meanwhile
this seems worth having on its ow.

Differential Revision: https://phabricator.services.mozilla.com/D186722
rayon-core/src/registry.rs Outdated Show resolved Hide resolved
@emilio emilio force-pushed the use-current-thread branch 2 times, most recently from d343448 to 10f1ee6 Compare September 4, 2023 13:32
emilio added a commit to emilio/rayon that referenced this pull request Sep 4, 2023
…oop.

This was originally done for rayon-rs#1063, in order to reuse this to allow
cleaning up the TLS data allocated by use_current_thread.

We ended up not using that, but this refactoring seems useful on its
own.
@emilio
Copy link
Contributor Author

emilio commented Sep 4, 2023

The PR / Format failure doesn't seem related (temporary github actions hiccup?)

rayon-core/src/lib.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented Sep 20, 2023

OK, I think we're good to go -- thanks for your persistence! :)

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 20, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 75524e2 into rayon-rs:master Sep 20, 2023
4 checks passed
cuviper pushed a commit to emilio/rayon that referenced this pull request Sep 20, 2023
…oop.

This was originally done for rayon-rs#1063, in order to reuse this to allow
cleaning up the TLS data allocated by use_current_thread.

We ended up not using that, but this refactoring seems useful on its
own.
emilio added a commit to emilio/rayon that referenced this pull request Sep 20, 2023
…oop.

This was originally done for rayon-rs#1063, in order to reuse this to allow
cleaning up the TLS data allocated by use_current_thread.

We ended up not using that, but this refactoring seems useful on its
own.
@emilio emilio deleted the use-current-thread branch September 20, 2023 19:02
@emilio
Copy link
Contributor Author

emilio commented Sep 20, 2023

OK, I think we're good to go -- thanks for your persistence! :)

Thank you!

bors bot added a commit that referenced this pull request Sep 20, 2023
1087: core: registry: Factor out "wait till out of work" part of the main loop. r=cuviper a=emilio

This was originally done for #1063, in order to reuse this to allow cleaning up the TLS data allocated by use_current_thread.

We ended up not using that, but this refactoring seems useful on its own, perhaps.

Co-authored-by: Emilio Cobos Álvarez <emilio@crisal.io>
@cuviper cuviper mentioned this pull request Jan 23, 2024
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.

3 participants