Skip to content

add array element mutex offset in print and gc #58997

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

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

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jul 14, 2025

The layout and printing logic need to correctly offset and align the inset fields to account for the per-element mutex of an atomic array with large elements.

Fix #58993

@vtjnash vtjnash requested a review from gbaraldi July 14, 2025 17:16
@vtjnash vtjnash added multithreading Base.Threads and related functionality backport 1.12 Change should be backported to release-1.12 labels Jul 14, 2025
@oscardssmith oscardssmith added the bugfix This change fixes an existing bug label Jul 14, 2025
@gbaraldi gbaraldi added the needs tests Unit tests are required for this change label Jul 14, 2025
@oscardssmith
Copy link
Member

Does this also need 1.11 backport?

@gbaraldi
Copy link
Member

julia> struct CxRL
           condvar::Condition
           x::Int
           lock::ReentrantLock
           CxRL() = new(Condition(), 5, ReentrantLock())
       end

julia> struct OnceHolder
           once::OncePerThread{CxRL}
           OnceHolder() = new(OncePerThread{CxRL}(CxRL))
       end

julia> holder = OnceHolder()
OnceHolder(OncePerThread{CxRL, Type{CxRL}}(CxRL[], UInt8[], CxRL))

julia> if length(ARGS) > 0
           display(holder)
       end

julia> holder.once()
CxRL(Condition(), 5, ReentrantLock())

julia> holder.once.xs[2]

This still hangs

vtjnash added 2 commits July 15, 2025 12:58
The layout and printing logic need to correctly offset and align the
inset fields to account for the per-element mutex of an atomic array
with large elements.

Fix #58993
@vtjnash
Copy link
Member Author

vtjnash commented Jul 15, 2025

Alright, now includes the additional revert required to fix that bug too.

@gbaraldi
Copy link
Member

The only issue with reverting that is a performance regression due to llvm's cost modeling pessimizing vectorization if the GEP calculation happens outside the GEP (it's their fault and maybe they will fix this in the future but this makes a bunch of operations regress) #57389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.12 Change should be backported to release-1.12 bugfix This change fixes an existing bug multithreading Base.Threads and related functionality needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory corruption, segmentation fault, with OncePerThread, Condition, ReentrantLock on v1.12.0-rc1, nightly
4 participants