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

task: schedule sticky tasks correctly with _wait2 #42750

Merged
merged 2 commits into from
Oct 22, 2021
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Oct 21, 2021

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)

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)
base/task.jl Outdated Show resolved Hide resolved
Co-authored-by: Takafumi Arakaki <[email protected]>
Copy link
Member

@tkf tkf left a 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?

@vtjnash
Copy link
Member Author

vtjnash commented Oct 21, 2021

The code shouldn't need to be duplicated more, and apparently doesn't have a good name for splitting out in that helper

@tkf
Copy link
Member

tkf commented Oct 21, 2021

Yeah, it was hard to name it and I also don't like the name. I don't have a strong opinion on refactoring.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Oct 21, 2021
Copy link
Member

@vchuravy vchuravy left a 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 :)

@DilumAluthge DilumAluthge merged commit 6420885 into master Oct 22, 2021
@DilumAluthge DilumAluthge deleted the jn/sticky-wait2 branch October 22, 2021 05:24
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Oct 22, 2021
KristofferC pushed a commit that referenced this pull request Oct 22, 2021
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)
c42f added a commit that referenced this pull request Nov 6, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
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]>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
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]>
Comment on lines +317 to +319
current_task().sticky = true
tid = Threads.threadid()
ccall(:jl_set_task_tid, Cvoid, (Any, Cint), waiter, tid-1)
Copy link
Contributor

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?

Copy link
Member Author

@vtjnash vtjnash May 1, 2023

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).

Copy link
Contributor

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 Conditions. t is a Task here, not a Condition. The semantics of this _wait2 seem unrelated to current_task().

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.

6 participants