Likely erroneous use of Threads.nthreads and Threads.threadid

[KristofferC]

Hello,

From the following PkgEval log: https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/5da257d_vs_d63aded/EnergyExpressions.primary.log it seems likely that this package is using Threads.nthreads and Threads.threadid incorrectly as described in https://julialang.org/blog/2023/07/PSA-dont-use-threadid/ and https://juliafolds2.github.io/OhMyThreads.jl/stable/literate/tls/tls/#The-naive-(and-incorrect)-approach.

In the upcoming Julia 1.12, julia runs with an interactive thread by default which means that the number of worker threads nthreads() defaults to 1 but the thread id that things will run on in a threaded context will typically be 2 (the interactive thread having the id 1):

julia> Threads.nthreads()
1

julia> Threads.@threads for i = 1:3
           @show Threads.threadid()
       end
Threads.threadid() = 2
Threads.threadid() = 2
Threads.threadid() = 2

Trying to index a vector of length Threads.nthreads() with Threads.threadid() will thus error.

Note that even though this code only started directly erroring in 1.12 the code was likely already incorrect on earlier Julia versions unless it made sure that the spawned tasks did not migrate between threads (which can happen at any yield point).

Some remedies to this are:

• Use the new OncePerThread (or OncePerTask) functionality added in 1.12.
• Use task local values instead of thread local values as described in https://juliafolds2.github.io/OhMyThreads.jl/stable/literate/tls/tls/#TLV
• Use a Channel as described in https://juliafolds2.github.io/OhMyThreads.jl/stable/literate/tls/tls/#The-safe-way:-Channel.
• Use Threads.maxthreadid() instead of Threads.nthreads(). This can waste some memory since maxthreadid() can give a higher number than the number of worker threads. It also doesn't help against the task migration issue described earlier (but the previous code did not do that either).

[jagot]

Thanks for the raising the issue! I have a feeling that this is already fixed on the branch threading-channels, which I am myself using at the moment, but have not merged because I have not written any tests, and have been busy recently actually doing more of physics using this code and less programming :)

Will try to get around to it over the next few weeks.