Avoid the zeros(nthreads())[threadid()] buffering pattern

[Drvi]

As recommended in this blogpost https://julialang.org/blog/2023/07/PSA-dont-use-threadid/

[etiennedeg]

This is known issue: #10
We already moved the algorithms causing failing tests in Experimental, but maybe all these algorithms should be in Experimental, as we can't ensure the correctness of these algorithms.

[gdalle]

Where do you think there might be other such algorithms?

[etiennedeg]

This should only be in Parallel, if it is somewhere else, I would be really worried. My concern is if we sufficiently advertise these Parallel algorithms as error-prone. For the moment, our take is: we never see the tests fail so we assume it works, though we know the code in clunky and might have race conditions that lead to potential errors.

[Drvi]

Hi folks and sorry for the delay in my reply. To clarify, this aims to fix an issue we might get into on 1.9.3 where multithreading behavior changed slightly in that Threads.nthreads() gives you the number of threads in the default threadpool, but the default thread ids are enumerated sequentially after the interactive threads, so with julia193 -t7,2, nthreads() == 7 and thread ids in the default thread pool are 3, 4, ..., 8, 9, e.g:

julia> buf = zeros(Threads.nthreads())
7-element Vector{Float64}:
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0

julia> Threads.@threads for i in 1:Threads.nthreads()
           buf[Threads.threadid()] = Threads.threadid()
       end
ERROR: TaskFailedException

    nested task error: BoundsError: attempt to access 7-element Vector{Float64} at index [9]
    Stacktrace:
     [1] setindex!(A::Vector{Float64}, x::Int64, i1::Int64)
       @ Base ./array.jl:969
     [2] macro expansion
       @ ./REPL[2]:2 [inlined]
     [3] (::var"#26#threadsfor_fun#4"{var"#26#threadsfor_fun#3#5"{UnitRange{Int64}}})(tid::Int64; onethread::Bool)
       @ Main ./threadingconstructs.jl:200
     [4] #26#threadsfor_fun
       @ ./threadingconstructs.jl:167 [inlined]
     [5] (::Base.Threads.var"#1#2"{var"#26#threadsfor_fun#4"{var"#26#threadsfor_fun#3#5"{UnitRange{Int64}}}, Int64})()
       @ Base.Threads ./threadingconstructs.jl:139

...and 1 more exception.

To avoid that, instead of Threads.threadid() we are using task ids, simple integer values we increment for each task we @spawn.

I don't know if this PR fixes any other, older known bugs or issues though.

[gdalle]

Thanks for the PR! Do you know why tests are failing?

[codecov]

Codecov Report

Merging #293 (56e8032) into master (1cb789d) will decrease coverage by 0.04%.
Report is 5 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
- Coverage   97.30%   97.27%   -0.04%     
==========================================
  Files         115      115              
  Lines        6709     6778      +69     
==========================================
+ Hits         6528     6593      +65     
- Misses        181      185       +4     

[Drvi]

@gdalle I think I fixed the issue 👍

[gdalle]

will review soon, thanks!

[gdalle]

Looks good to me