-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
task: schedule sticky tasks correctly with _wait2 #42750
Conversation
The `_wait2` function is similar to calling `schedule + wait` from another `@async` task, but optimized, so we want to observe the same side-effects of making the task sticky to the correct thread (and not accidentally making it sticky to the later task that handles the event). Refs #41334 (75858d7)
Co-authored-by: Takafumi Arakaki <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh... this is unfortunate but makes sense to me. How about adding something like
"""
_coschedule_parent_if_required(t::Task) -> Bool
Make `current_task()` sticky if `t` is a newly scheduled sticky task and return `true`.
t.sticky && threadid(t) == 0 is a task that needs to be co-scheduled with
the parent task. If the parent (current_task) is not sticky we must
set it to be sticky.
XXX: Ideally we would be able to unset this
"""
function _coschedule_parent_if_required(t::Task)
(t.sticky && Threads.threadid(t) == 0) || return false
current_task().sticky = true
tid = Threads.threadid()
ccall(:jl_set_task_tid, Cvoid, (Any, Cint), t, tid-1)
return true
end
to avoid repeating the tricky code?
The code shouldn't need to be duplicated more, and apparently doesn't have a good name for splitting out in that helper |
Yeah, it was hard to name it and I also don't like the name. I don't have a strong opinion on refactoring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record. I hate this :)
The `_wait2` function is similar to calling `schedule + wait` from another `@async` task, but optimized, so we want to observe the same side-effects of making the task sticky to the correct thread (and not accidentally making it sticky to the later task that handles the event). Refs #41334 (75858d7) Co-authored-by: Takafumi Arakaki <[email protected]> (cherry picked from commit 6420885)
The `_wait2` function is similar to calling `schedule + wait` from another `@async` task, but optimized, so we want to observe the same side-effects of making the task sticky to the correct thread (and not accidentally making it sticky to the later task that handles the event). Refs JuliaLang#41334 (75858d7) Co-authored-by: Takafumi Arakaki <[email protected]>
The `_wait2` function is similar to calling `schedule + wait` from another `@async` task, but optimized, so we want to observe the same side-effects of making the task sticky to the correct thread (and not accidentally making it sticky to the later task that handles the event). Refs JuliaLang#41334 (75858d7) Co-authored-by: Takafumi Arakaki <[email protected]>
current_task().sticky = true | ||
tid = Threads.threadid() | ||
ccall(:jl_set_task_tid, Cvoid, (Any, Cint), waiter, tid-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vtjnash and @vchuravy: I think this is wrong. Here, waiter
is waiting on t
, not on current_task()
which can be completely different (e.g. see bind).
Apparently, we want to pin t
and waiter
to the same thread. But I'm not sure how this follows from the original problem: #41324. It's perfectly reasonable for a task in the interactive threadpool to wait on a task in the default threadpool, but this makes that unachievable.
I know it's been a while, but can either of you remember why this was added after #41334?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_wait2
is defined as semantically a call to schedule
the Task waiter
to immediately call wait
on some condition variable t
. Hence why it has the same side-effects as calling schedule
: it should pin current_task
and waiter
to one thread, but not t
(which will have completed before waiter
runs anyways).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That task t
will have completed before waiter
runs makes sense. But this _wait2
as opposed to the one in condition.jl
has nothing to do with Condition
s. t
is a Task
here, not a Condition
. The semantics of this _wait2
seem unrelated to current_task()
.
The
_wait2
function is similar to callingschedule + wait
fromanother
@async
task, but optimized, so we want to observe the sameside-effects of making the task sticky to the correct thread (and not
accidentally making it sticky to the later task that handles the event).
Refs #41334 (75858d7)