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

Fix enq_work behavior when single-threaded #48702

Merged
merged 5 commits into from
Mar 9, 2023
Merged

Fix enq_work behavior when single-threaded #48702

merged 5 commits into from
Mar 9, 2023

Conversation

kpamnany
Copy link
Contributor

@kpamnany kpamnany commented Feb 17, 2023

If there's only one thread in the task's preferred thread pool, use that thread's work queue.

Fixes #48644.
Fixes #47756

$ ./julia -t 1,1
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.10.0-DEV.628 (2023-02-17)
 _/ |\__'_|_|_|\__'_|  |  kp/fix-48644/2420ad1286 (fork: 1 commits, 0 days)
|__/                   |

julia> fetch(Threads.@spawn :interactive Threads.threadpool())
:interactive

julia>

@giordano giordano added domain:multithreading Base.Threads and related functionality backport 1.9 Change should be backported to release-1.9 labels Feb 17, 2023
Copy link
Sponsor 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.

I don't think this is quite right either. I think the issue is that where we set tid = Threads.threadid(), it should first check if t has a preferred thread pool instead, instead of pinning it to the current thread.

A similar change will be needed in the other enq_work-like functions (e.g. wait2)

@KristofferC KristofferC mentioned this pull request Feb 20, 2023
50 tasks
@kpamnany
Copy link
Contributor Author

I think the issue is that where we set tid = Threads.threadid(), it should first check if t has a preferred thread pool instead, instead of pinning it to the current thread.

My understanding of the logic is as follows.

If maxthreadid() is 1, the multiqueue is never used so all tasks go to the single thread's work queue whether they are sticky or not.

Otherwise, if t is sticky but has no tid, it is stuck to the current thread and the current task is also made sticky.

If t has a thread pool, we can stick it to one of that pool's threads, but what do we do with the current task (which is already running on the current thread)?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 21, 2023

maxthreadid is dynamic, so should not be used for static scheduling decisions is the main challenge

@kpamnany
Copy link
Contributor Author

This particular case is not a static scheduling decision though? If maxthreadid() were to increase from 1, the task's stickiness starts to matter, so subsequent scheduling decisions will be different, as they should be. Not seeing the problem.

Currently, the only way to create a task on a different thread pool is with @spawn, and such tasks are not sticky. So it should not be possible for t to be sticky and have a different thread pool from the caller.

@simsurace
Copy link
Contributor

Is the issue that the threadpool size cannot be determined for the interactive thread pool if this pool does not exist? If an interactive threadpool is not available, what should happen when spawning a task on it? Should this error or simply revert to the default pool?

base/task.jl Outdated
@@ -767,7 +767,7 @@ end

function enq_work(t::Task)
(t._state === task_state_runnable && t.queue === nothing) || error("schedule: Task not runnable")
if t.sticky || Threads.threadpoolsize() == 1
if t.sticky || Threads.maxthreadid() == 1
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
if t.sticky || Threads.maxthreadid() == 1
if !t.sticky
tpid = max(threadpoolid(t), 1)
if tpid == 1 && Threads.threadpoolsize(tpid) == 1
ccall(:jl_set_task_tid, Cint, (Any, Cint), t, 1)
t.sticky = true
end
end
if t.sticky

I think this was trying to check specifically the size of the first threadpool, and check if t is meant to run in the first threadpool, and if both are 1, then to add it to the sticky queue for the first threadpool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reworked this to check for the size of the task's preferred thread pool, whichever it is.

base/task.jl Show resolved Hide resolved
Allow specifying which thread pool's size to retrieve.
Now returns `threadpoolsize(pool)`.
If a task is spawned with `:interactive` but there are no interactive
threads, set the task's thread pool to `:default` so that we don't have
to keep checking it in other places.
@kpamnany kpamnany changed the title Fix test for single-threadedness Fix enq_work behavior when single-threaded Mar 6, 2023
@kpamnany
Copy link
Contributor Author

kpamnany commented Mar 6, 2023

Is the issue that the threadpool size cannot be determined for the interactive thread pool if this pool does not exist? If an interactive threadpool is not available, what should happen when spawning a task on it? Should this error or simply revert to the default pool?

Sorry, forgot to answer this. The recent commits should make it clearer, but basically if there's only one thread in whichever thread pool a task is to run in, we want to behave as though that task is sticky (i.e. use the thread's work queue rather than the multiqueue) as a small optimization.

Your other question should also be answered by one of the commits here -- if there is no interactive thread pool, we simply revert to the default thread pool.

base/task.jl Outdated Show resolved Hide resolved
If there's only one thread in the task's preferred thread pool, use that
thread's work queue.
@kpamnany
Copy link
Contributor Author

kpamnany commented Mar 9, 2023

Is this done then @vtjnash?

@vtjnash vtjnash merged commit f288f88 into master Mar 9, 2023
@vtjnash vtjnash deleted the kp/fix-48644 branch March 9, 2023 19:28
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:multithreading Base.Threads and related functionality
Projects
None yet
5 participants