Skip to content

add @threads static to aid thread API evolution #35646

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

Merged
merged 1 commit into from
May 5, 2020

Conversation

JeffBezanson
Copy link
Member

Looking into stabilizing the thread API in 1.5, I think one major likely future change is the schedule used by @threads. Eventually we want it to spawn un-pinned tasks (and possibly more tasks than threads), yet we know some code is already using it in a way that assumes the current schedule. I think a good approach is to allow @threads static now, so that code depending on the schedule can be future-proofed, and then in a later version we can change the default schedule.

@JeffBezanson JeffBezanson added the multithreading Base.Threads and related functionality label Apr 29, 2020
@tkf
Copy link
Member

tkf commented Apr 29, 2020

Why not sunset @threads and instead implement Threads.foreach for array-like inputs? I think it's nice to be transparent that a threaded loop is going to create a closure.

Comment on lines 105 to 106
The only currently supported value is `static`, which creates one task per thread
and divides the iterations equally among them.
Copy link
Member

Choose a reason for hiding this comment

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

This is not actually true, right? It only holds when Threads.@threads is used in the main thread. This still works (before and after #35632):

julia> Threads.@threads for i in 1:Threads.nthreads()
           @assert i == Threads.threadid()
           Threads.@threads for j in 1:Threads.nthreads()
               if i != 1
                   @assert i == Threads.threadid()  # inherits parent tid
               end
           end
       end

How about something like

Suggested change
The only currently supported value is `static`, which creates one task per thread
and divides the iterations equally among them.
The only currently supported value is `static`, which creates tasks as many as available
threads and divides the iterations equally among them.

?

As for "one task per thread" API, I think what people actually need is Threads.pin.

@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Apr 30, 2020
@JeffBezanson
Copy link
Member Author

Question for triage: as tkf said, the static schedule is not exactly what's documented. The current behavior is that we spawn 1 task per thread when @threads is called from thread 1, otherwise all iterations run on the current thread. Should we add the behavior that nested calls to @threads on thread 1 also disable parallelism?

@JeffBezanson
Copy link
Member Author

From triage: disable parallelism when nested.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Apr 30, 2020
@tkf
Copy link
Member

tkf commented Apr 30, 2020

How about

julia> const STOP = Threads.Atomic{Bool}(false)
       Threads.@spawn while !STOP[] end
       Threads.@threads for i in 1:Threads.nthreads()
           @show i
       end
       STOP[] = true
i = 3
i = 1
i = 4
^C

? It hangs currently.

@JeffBezanson
Copy link
Member Author

That's an example of why I think @threads should use a more dynamic schedule by default, but with the static schedule I don't see what else it can do. What should it do?

@tkf
Copy link
Member

tkf commented Apr 30, 2020

Why not just

basesize = cld(length(xs), Threads.nthreads())
@sync for p in Iterators.partition(xs, basesize)
    @spawn foreach(loop_body, p)
end

? It's "static" in the sense that the sub-chunk of the array to be processed by each task is determined by nthreads() and size(xs).

IIUC, any API that would specify the assignment to thread would be impossible to compose well in task-based parallelism. Some kind of API for this (e.g., Threads.pin) may be useful for power users but I don't think a high-level API like @threads should have this. Maybe at least use an ugly name like unsafe_static or legacy_static?

@JeffBezanson
Copy link
Member Author

IIUC, any API that would specify the assignment to thread would be impossible to compose well in task-based parallelism.

Yes, that is completely true. Some existing code assumes it though. And I believe the pinned thread assignments are part of what's meant by a "static" schedule in standard terminology. I guess I'm ok with giving it an uglier name though.

@tkf
Copy link
Member

tkf commented Apr 30, 2020

To support existing code bases, wouldn't it be even nicer to just throw an error when Threads.threadid() != 1 (or whenever the static assignment is impossible)? If they assume static scheduling, I suppose it's safer to have an exception rather than silently corrupting their computation? We can "guarantee" static assignment this way (as otherwise the program either deadlocks or throws). It's a bit harsh API but I don't think it's a huge problem if we introduce something like Iterators.partition-based scheduler at the same time and make it default. It's not great that we can't provide a grace period for those who are assuming static scheduling. But I don't think it's possible to not break their code at some point anyway.

@StefanKarpinski
Copy link
Member

Why do so much work done to have a composable threading system only to refuse to allow threads to compose?!? If we wanted nested @threads calls to fail, we could have had that five years ago.

@tkf
Copy link
Member

tkf commented Apr 30, 2020

I'm proposing default @threads for to use non-static scheduling so that it composes well when nested in @threads or @spawn. The extra work is for allowing existing code to still use static scheduling when they happened to assume it. Code assuming static scheduling won't compose well anyway so I don't know what else can be done to rescue them.

@StefanKarpinski
Copy link
Member

Code assuming static scheduling won't compose well anyway so I don't know what else can be done to rescue them.

Just run it sequentially? It's pretty hard to write code using @threads that wouldn't also be correct, if slower, if you just ran it sequentially on one thread. You can't always control how your code is called or even what your code calls, if it's generic. Just failing seems like the worst option.

@JeffBezanson
Copy link
Member Author

We also need to remove _threadedregion (#32477) for the full story to work anyway.

@tkf
Copy link
Member

tkf commented Apr 30, 2020

Just run it sequentially?

I guess it depends on how strong their assumption is and how much you want to allow it? Falling back to sequential execution does not help a case like

const VECTORS = Vector{Vector{Float64}}[]
__init__() = Threads.resize_nthreads!(empty!(VECTORS), Float64[])

function f(outputs, inputs, callback)
    @threads for (y, x) in zip(outputs, inputs)
        tmp = VECTORS[Threads.nthreads()]
        compute_1!(tmp, x)
        callback()
        compute_2!(y, x, tmp)
    end
end

when f itself is called via callback. I think failing is much better in this case.

@JeffBezanson
Copy link
Member Author

I guess we could give an error for nested @threads if you specify static? Maybe that's what you meant and I missed it. Because in general we can assume that a user of @threads doesn't care what schedule is used.

@tkf
Copy link
Member

tkf commented Apr 30, 2020

Ah, yes, that's what I meant. Sorry, it wasn't clear.

# static schedule
function _atthreads_static_schedule()
ids = zeros(Int, nthreads())
Threads.@threads static for i = 1:nthreads()
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a stupid question: of which type is static in here? It’s not a symbol and looks like a keyword or a variable instead. As the tests are passing, I assume it’s not a typo.

I would like to propose that it should be a symbol instead, so that the scheduling may be controlled by a variable or similar (cf. @spawnat :any ... and @spawnat 2 ... from Distributed).

Copy link
Member

Choose a reason for hiding this comment

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

It's used only at macro-expansion time so it doesn't correspond to any variables. We already have something like @simd ivdep for ... end so I think it's OK to have this interface.

But I think using :static actually makes sense. It's handy to decide scheduler at run-time like

function f(...; scheduler=:static)
    @threads scheduler for ... end
end

Copy link
Member Author

Choose a reason for hiding this comment

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

It's handy to decide scheduler at run-time

This is actually a really fundamental decision though. Is the scheduler an object from an expression evaluated at run time, or is it known at macro expansion time, potentially allowing the macro to emit different code for different schedules?

Copy link
Member

Choose a reason for hiding this comment

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

Do you think most (all?) of the schedulers need to do something during macro expansion? Perhaps that's the reason why you are not in favor of Threads.foreach approach?

I was actually thinking that a (large) subset of schedulers can be set at run-time. We can throw a run-time error when a scheduler that has to be expanded by the macro is specified at runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably true, most schedules could be picked at run-time. I guess it's a good compromise to in general evaluate the expression, but recognize certain constant expressions like :static at macro expand time. I'll change it to that.

The default schedule (used when no `schedule` argument is present) is subject to change.

!!! compat "Julia 1.5"
The `schedule` argument is available as of Julia 1.5.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The `schedule` argument is available as of Julia 1.5.
The `schedule` argument is available as of Julia 1.5.
Nesting `@threads` calls now throws an exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only if you specify :static; I fixed that in the doc string.

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.

5 participants