Skip to content

If the user explicitly asked for 1 thread don't add an interactive one. #57454

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

Merged
merged 12 commits into from
Jun 4, 2025

Conversation

gbaraldi
Copy link
Member

This basically makes the assumption that if the user asked for 1 thread they really want their program to not have threads

This basically makes the assumption that if the user asked for 1 thread they really want their program to not have threads
@IanButterworth
Copy link
Member

IanButterworth commented Feb 18, 2025

As this introduces a complexity to the setting, I think this need the docs to be updated to understand how clear this would be i.e. both -t and JULIA_NUM_THREADS behavior

@giordano giordano added multithreading Base.Threads and related functionality backport 1.12 Change should be backported to release-1.12 labels Feb 18, 2025
@topolarity topolarity requested a review from xal-0 February 18, 2025 16:41
@IanButterworth

This comment was marked as resolved.

@KristofferC KristofferC mentioned this pull request Feb 21, 2025
24 tasks
@IanButterworth IanButterworth added the needs tests Unit tests are required for this change label Feb 21, 2025
Copy link
Member

@IanButterworth IanButterworth left a comment

Choose a reason for hiding this comment

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

Makes sense to me, just needs a test alongside the ones that check threadpool sizes.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

This sort of thing seems very unintuitive. Why are we getting hidden unexplained threads at all when the user didn't ask for them?

@IanButterworth
Copy link
Member

IanButterworth commented Feb 22, 2025

Because the default number of interactive threads changed. See motivation in #57087

From discussion in the multithreading call there was general agreement that while this is complication of a simple rule, -t1 does seem like a case worth special casing:

-t2 : most likely you would benefit from separating main task (& io loop), and tasks on worker threads.
-tauto: same as above
-t1: you probably want specifically one thread because otherwise you'd just leave the default -t1,1

This was referenced Mar 24, 2025
@KristofferC KristofferC mentioned this pull request Apr 4, 2025
51 tasks
@KristofferC KristofferC mentioned this pull request Apr 29, 2025
53 tasks
@gbaraldi gbaraldi removed the needs tests Unit tests are required for this change label May 6, 2025
@KristofferC KristofferC mentioned this pull request May 9, 2025
58 tasks
@IanButterworth
Copy link
Member

Tests should pass once #58619 lands

@IanButterworth IanButterworth added the merge me PR is reviewed. Merge when all tests are passing label Jun 3, 2025
@IanButterworth IanButterworth merged commit b04b104 into JuliaLang:master Jun 4, 2025
6 of 8 checks passed
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jun 4, 2025
@KristofferC KristofferC mentioned this pull request Jun 6, 2025
60 tasks
@topolarity
Copy link
Member

@gbaraldi Can you assist with backporting this?

@gbaraldi
Copy link
Member Author

gbaraldi commented Jul 3, 2025

Yeah, I imagined this would apply quite cleanly

KristofferC pushed a commit that referenced this pull request Jul 4, 2025
…e. (#57454)

Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
(cherry picked from commit b04b104)
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants