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

Unstick parent if all child tasks are done #41393

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

tkf
Copy link
Member

@tkf tkf commented Jun 28, 2021

This is a possible improvement built upon #41334 implementing the idea I mentioned in #41324 (comment)

fixes #41324

@tkf tkf added the multithreading Base.Threads and related functionality label Jun 28, 2021
src/julia.h Outdated
@@ -1829,8 +1829,8 @@ typedef struct _jl_task_t {
jl_value_t *result;
jl_value_t *logstate;
jl_function_t *start;
uint16_t sticky; // 0 means this Task can be migrated to a new thread
Copy link
Member Author

Choose a reason for hiding this comment

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

uint16 here is somewhat arbitrary. It works with uint8 too (see the saturation check) but I thought 255 tasks are not super crazily many tasks.

@JeffBezanson
Copy link
Sponsor Member

This looks like it could have a lot of overhead. Is a feature like this truly needed long term, or should we just encourage making more things thread-safe?

@tkf
Copy link
Member Author

tkf commented Jun 29, 2021

should we just encourage making more things thread-safe?

Oh yes, I'd love to go this direction :)

But I thought we'd need to keep living with code @async for a while. So, this PR is just another shot at avoiding affected by the code using @async. Since the workaround by #41334 can kick in even when transitively calling function using @async, I thought it might be better to try undo the workaround whenever possible.

This looks like it could have a lot of overhead.

What's the main overhead you are thinking? I think there are:

  1. Additional dynamic dispatch due to f.code::Any.
  2. Increments/decrements of the counter. (Dominated by store?)

I thought they might not be so prominent compared to a task creation + scheduling overhead.

@vchuravy vchuravy requested a review from vtjnash June 29, 2021 08:51
@JeffBezanson
Copy link
Sponsor Member

I thought they might not be so prominent compared to a task creation + scheduling overhead.

Right, the overhead is currently way way too high, so I don't want to add any more to it :)

@tkf
Copy link
Member Author

tkf commented Jun 29, 2021

Yes, it's an overhead and code complexity. But I'm thinking a situation like a @spawned task uses @async (transitively) in a short preparation step and then does some long-running number crunching (maybe with occasional synchronizations with other tasks) afterward. I think the performance gain of the task migration beats the overhead of the sticky counter in this case. I think users' frustration from cases like this is more common than the performance of @async itself since, presumably, people use @async when they don't care too much about performance (which can be gained by using @spawn).

That said, I don't think #41334 is too bad since that's another motivation for us to recommend @spawn over @async everywhere.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 29, 2021

I suppose the risk is that people might write algorithms (e.g. spinlocks) that depend, for deadlock-free correctness, on the ability to migrate threads, and then adding a @async call will disrupt that, which might mean we need this PR. But then #39773 (RFC: Experimental API for may-happen in parallel parallelism) wants to adopt a memory model that says such programs are instead triggering of UB, which might suggest we do not need this PR?

@tkf
Copy link
Member Author

tkf commented Jun 29, 2021

#39773 restricts the use of communication APIs like channel, condition variable, and synchronization barrier though. So, if we want to enable task migration for such "concurrent" tasks as much as possible, I think this PR is better than #41334.

the risk is that people might write algorithms (e.g. spinlocks) that depend, for deadlock-free correctness, on the ability to migrate threads

Bit aside, but writing something like this hasn't been possible as there was not migration, right? How about documenting such pattern is unsupported? (Though Hyrum's law may be triggered anyway...)

base/task.jl Outdated Show resolved Hide resolved
else
return getfield(t, field)
end
end

function setproperty!(t::Task, field::Symbol, x)
if field === :sticky
t.sticky_count = convert(Bool, x)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what prevents an @async-scheduled task from being marked as unsticky since sticky_count will always be >= 1, right? Maybe it would be good to add comment here. Or a test.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is rather a compat layer for supporting .sticky property.

@async task is never marked as unsticky because (1) its sticky_count is initialized to 1 (in the C function jl_new_task) and (2) a decrement (if any) is always paired with a preceding increment.

Co-authored-by: Jonas Schulze <[email protected]>
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.

@async tasks executes simultaneously with parent task if launched with @spawn
5 participants