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

threading deadlock: change jl_fptr_wait_for_compiled to actually compile code #56571

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 15, 2024

The jl_fptr_wait_for_compiled state merely means it could compile that code, but in many circumstances, it will not actually compile that code until a long delay: when either all edges are satisfied or it is demanded to run immediately. The previous logic did not handle that possibility leading to deadlocks (possible even on one thread).

A high rate of failure was shown on running the following CI test:

$ ./julia -t 20 -q <<EOF
for i = 1:1000
i % 10 == 0 && @show i
(@eval function _atthreads_greedy_dynamic_schedule()
    inc = Threads.Atomic{Int}(0)
    Threads.@threads :greedy for _ = 1:Threads.threadpoolsize()
        Threads.@threads :dynamic for _ = 1:Threads.threadpoolsize()
            Threads.atomic_add!(inc, 1)
        end
    end
    return inc[]
end)() == Threads.threadpoolsize() * Threads.threadpoolsize()
end
EOF

@vtjnash vtjnash requested a review from gbaraldi November 15, 2024 15:47
@gbaraldi
Copy link
Member

LGTM with analyze GC happier

@vtjnash vtjnash force-pushed the jn/wait_for_compiled-deadlock branch from 1f3fcde to 6c458db Compare November 15, 2024 17:02
…ile code

The jl_fptr_wait_for_compiled state merely means it could compile that
code, but in many circumstances, it will not actually compile that code
until a long delay: when either all edges are satisfied or it is
demanded to run immediately. The previous logic did not handle that
possibility leading to deadlocks (possible even on one thread).

A high rate of failure was shown on running the following CI test:

    $ ./julia -t 20 -q <<EOF
    for i = 1:1000
    i % 10 == 0 && @show i
    (@eval function _atthreads_greedy_dynamic_schedule()
        inc = Threads.Atomic{Int}(0)
        Threads.@threads :greedy for _ = 1:Threads.threadpoolsize()
            Threads.@threads :dynamic for _ = 1:Threads.threadpoolsize()
                Threads.atomic_add!(inc, 1)
            end
        end
        return inc[]
    end)() == Threads.threadpoolsize() * Threads.threadpoolsize()
    end
    EOF
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Nov 15, 2024
@vtjnash vtjnash force-pushed the jn/wait_for_compiled-deadlock branch from 6c458db to 1d331d1 Compare November 15, 2024 17:17
@vtjnash vtjnash merged commit fa880a7 into master Nov 18, 2024
4 of 7 checks passed
@vtjnash vtjnash deleted the jn/wait_for_compiled-deadlock branch November 18, 2024 15:41
@oscardssmith oscardssmith added multithreading Base.Threads and related functionality bugfix This change fixes an existing bug and removed merge me PR is reviewed. Merge when all tests are passing labels Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants