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

Default to 1 interactive thread #57087

Merged

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Jan 17, 2025

Part of #43672

So:

  • default == 1 default, 1 interactive
  • -t1 == 1 default, 1 interactive
  • -t1,0 == 1 default, 0 interactive
    etc.

Motivations

  • Means the IO loop isn't on the worker threadpool (unless interactive threadpool is set to 0)
  • Allows the REPL to do threaded things in the background i.e.
    • completion hints that could be cancelled on next keystroke
    • Pkg loads in the background so user can type immediately in the pkg repl

Todo

  • Docs & News
    • Make it clear that the repl operates in the :interactive threadpool, and that a bare @spawn or @threads will schedule in the :default threadpool
    • Cover that the IO loop by default now runs in a different thread to operations
    • Explain how to disable it. i.e. -t1,0
    • Explain no :interactive threadpool during precompilation/PackageCompiler

Issues

@IanButterworth IanButterworth added the multithreading Base.Threads and related functionality label Jan 17, 2025
src/jloptions.c Outdated Show resolved Hide resolved
@inkydragon

This comment was marked as resolved.

@IanButterworth IanButterworth force-pushed the ib/default_interactive_thread branch 2 times, most recently from bd7417f to ac4361b Compare January 18, 2025 15:27
@IanButterworth IanButterworth added needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Jan 18, 2025
Comment on lines -537 to -550
function test_thread_range()
a = zeros(Int, threadpoolsize())
@threads for i in 1:threadid()
a[i] = 1
end
for i in 1:threadid()
@test a[i] == 1
end
for i in (threadid() + 1):threadpoolsize()
@test a[i] == 0
end
end
test_thread_range()

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't figure out what this test is actually testing, given the test script always runs from thread 1

@IanButterworth IanButterworth force-pushed the ib/default_interactive_thread branch 5 times, most recently from 0eaf9c1 to 4b52062 Compare January 19, 2025 04:56
@IanButterworth IanButterworth force-pushed the ib/default_interactive_thread branch 2 times, most recently from 339c71f to 50a8cce Compare January 20, 2025 03:51
@@ -614,7 +614,7 @@ Start some other operations that use `f(x)`:
julia> g(x) = f(x)
g (generic function with 1 method)
julia> t = Threads.@spawn f(wait()); yield();
julia> t = @async f(wait()); yield();
Copy link
Member Author

Choose a reason for hiding this comment

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

These were changed to @spawn in #55315 but I think they make sense as @async otherwise more orchestration is needed when multiple threads are available to make the examples work as expected.

Comment on lines +603 to +608
// single threaded mode
// Note: with -t1,1 a signal 10 occurs in task_scanner
jl_options.nthreadpools = 1;
jl_options.nthreads = 1;
int16_t ntpp[] = {jl_options.nthreads};
jl_options.nthreads_per_pool = ntpp;
Copy link
Member Author

Choose a reason for hiding this comment

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

Could the signal 10 be related to/fixed by #56477 ?

@IanButterworth IanButterworth force-pushed the ib/default_interactive_thread branch from fc001e6 to 920085e Compare January 21, 2025 03:06
@IanButterworth IanButterworth removed needs news A NEWS entry is required for this change needs docs Documentation for this change is required labels Jan 21, 2025
@IanButterworth
Copy link
Member Author

Building on @JamesWrigley's work we've done a push to more extensively test and fix Distributed's thread safety, including running its own tests with -t4,4 and enabling tests that were previously disabled when multithreading, which shook out issues that are now fixed #57169 There's room for improvement (moving away from @async to avoid races) but the situation seems evidently good now based on testing.

I believe that was the only blocking issue here?

I believe these make more sense as async than spawn, otherwise more orchestration is needed in the example.

Were changed to spawn in JuliaLang#55315
@IanButterworth IanButterworth force-pushed the ib/default_interactive_thread branch from 920085e to eb92c57 Compare January 27, 2025 19:58
@oscardssmith oscardssmith added the merge me PR is reviewed. Merge when all tests are passing label Jan 27, 2025
@IanButterworth IanButterworth merged commit fbe8656 into JuliaLang:master Jan 28, 2025
8 checks passed
@IanButterworth IanButterworth deleted the ib/default_interactive_thread branch January 28, 2025 03:44
@IanButterworth IanButterworth removed the merge me PR is reviewed. Merge when all tests are passing label Jan 28, 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.

3 participants