Skip to content

[DNM] remove thread safety #45

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[DNM] remove thread safety #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions src/Perms/perm_images.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
else
mutable struct Perm{T<:Integer} <: AP.AbstractPermutation
images::Vector{T}
@atomic inv::Perm{T}
@atomic cycles::AP.CycleDecomposition{T}
inv::Perm{T}
cycles::AP.CycleDecomposition{T}

function Perm{T}(images::Vector{T}; check::Bool = true) where {T}
if check && !isperm(images)
Expand Down Expand Up @@ -92,36 +92,36 @@ else
function Base.copy(p::Perm)
imgs = copy(p.images)
q = typeof(p)(imgs; check = false)
if isdefined(p, :inv, :sequentially_consistent)
inv_imgs = copy(@atomic(p.inv).images)
if isdefined(p, :inv)
inv_imgs = copy(p.inv.images)
q⁻¹ = typeof(p)(inv_imgs; check = false)
@atomic q.inv = q⁻¹
@atomic q⁻¹.inv = q
q.inv = q⁻¹
q⁻¹.inv = q
Comment on lines +98 to +99
Copy link

@vtjnash vtjnash Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You did have a bad mistake here (of generally the error sort, though generally not the crash sort), since you assigned these in the wrong order, the :sequentially_consistent annotation is invalidated everywhere and invalid.

Suggested change
q.inv = q⁻¹
q⁻¹.inv = q
@atomic q⁻¹.inv = q
@atomic q.inv = q⁻¹

end
return q
end

function Base.inv(σ::Perm)
if !isdefined(σ, :inv, :sequentially_consistent)
if !isdefined(σ, :inv)
if isone(σ)
@atomic σ.inv = σ
σ.inv = σ
else
σ⁻¹ = typeof(σ)(invperm(σ.images); check = false)
# we don't want to end up with two copies of inverse σ floating around
Copy link

Choose a reason for hiding this comment

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

This comment is not valid (doesn't describe the code / not physically possible). There is an atomic annotation for this (@atomiconce), but that doesn't actually fix the physics problem of not being able to do 2 writes simultaneously

Copy link
Owner

Choose a reason for hiding this comment

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

@vtjnash thanks for the comments!

Yeah, now after you pointed this, it seems obvious that I should have done

@atomic σ⁻¹.inv = σ # fully sets up the "local inverse"
@atomic σ.inv = σ⁻¹ # makes the "local inverse" visible to the global world.

so that σ is updated (atomically) only once 🤦🏻

I guess reading of σ.inv below should be also @atomic?

Copy link

Choose a reason for hiding this comment

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

It isn't super clear what you want or need here. I think you're just asking for a release store, since the reverse order has the opposite problem since they reference each other. Otherwise you'd have to use a global mutex (to actually implement the comment) and I'm unclear that you actually want that behavior? You also must use atomiconce macro if you want this only happen once, otherwise it will be atomic, but happen potentially as many times as there are threads

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine this happening multiple times, cause once the field is defined there will be only reads to it;

What I want to avoid is the situation where isdefined returns true, but the internal data structures are read in a not fully-initialized state (I have observed such bugs before). Here the other order could mean that another comes to p::Perm and sees that p.inv is defined so just takes it, but the thread computing p⁻¹ has not fully initialized it. Here it will only result in yet another computation of p⁻¹.inv. In other cases I got some random "Access to undefined reference" despite asking for isdefined.

Copy link

Choose a reason for hiding this comment

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

cause once the field is defined there will be only reads to it;

That is not quite what you implemented with @atomic, since you've only implemented that per-thread, so once the field is defined on a given thread, there will only be reads to it on that thread, but there will still be writes to it on all the other threads since you didn't specify that it should only be done once

Copy link
Owner

@kalmarek kalmarek Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm fine if two/many threads enter the body of if isdefined and then even overwrite the field, as long as the data in the field is consistent. I understand that I could do @atomiconce p.inv = p⁻¹ to specifically tell the "late" threads not to touch the field. Maybe that'd even better. But since all of them should compute exactly the same thing I don't think it makes a difference in this case.

To put it differently: Are these statements correct:

  1. Once the field of the object is defined any thread that has not yet entered the body of if isdefined(...) will not enter it (and hence will neither perform the computations nor overwrite the field).
  2. Any thread that has entered the body of if defined(...) before the initialization of the field will compute and override the field.

Copy link

Choose a reason for hiding this comment

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

Yes, that sounds like what you get from marking it with sequential consistency (though it can be slower than release-acquire which is all you really need). Though even without any atomic annotations, reference fields get implemented with release-consume, so these atomics don't appear that they would make a difference to the observable behaviors

Copy link
Owner

Choose a reason for hiding this comment

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

thanks for the clarification!
What do you suggest then instead of sequential_consistent?

    if isdefined(p, :inv, :acquire)
        inv_imgs = copy(@atomic(p.inv).images)
        q⁻¹ = typeof(p)(inv_imgs; check = false)
        @atomic q⁻¹.inv = q # which ordering here?
        @atomiconce :release q.inv = q⁻¹
    end

Hmm, I'm not sure again if I understand how this could be achieved without atomics. e.g. I had situation where on lazy initializations I was getting "Access to undefined field" despite using if isdefined(...). It turned out then that some of the fields were read with nullptrs despite isdefined returning true. These errors disappeared either by using locking (in some cases), or by using @atomic here...

My understanding of the fine details of multithreading is lacking, so I appreciate all of the comments of yours. They help me to clarify things in my head, thanks!

Copy link

Choose a reason for hiding this comment

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

:release or :monotonic would be fine there, since that is not globally visible memory. You might get a torn read/write if it is a struct that can be inlined which can cause things to get into an inconsistent state. Using @atomic explicitly is definitely more correct and better, even if it happens compiles down to the same cpu instructions though. I just note it so you can be alert to whether there are other possible synchronization issues that are being hidden by the seq_cst just making it run slower and being more rare to observe.

if !isdefined(σ, :inv, :sequentially_consistent)
@atomic σ.inv = σ⁻¹
@atomic σ⁻¹.inv = σ
if !isdefined(σ, :inv)
σ.inv = σ⁻¹
σ⁻¹.inv = σ
end
end
end
return σ.inv
end

function AP.cycles(σ::Perm)
if !isdefined(σ, :cycles, :sequentially_consistent)
if !isdefined(σ, :cycles)
cdec = AP.CycleDecomposition(σ)
# we can afford producing more than one cycle decomposition
@atomic σ.cycles = cdec
σ.cycles = cdec
end
return σ.cycles
end
Expand Down
4 changes: 2 additions & 2 deletions src/group_interface.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ Base.one(G::PermGroup{P}) where {P} = Permutation(one(P), G)
end
else
function GroupsCore.order(::Type{T}, G::AbstractPermutationGroup) where {T}
if !isdefined(G, :order, :sequentially_consistent)
if !isdefined(G, :order)
ord = order(StabilizerChain(G))
@atomic G.order = ord
G.order = ord
end
return convert(T, G.order)
end
Expand Down
10 changes: 5 additions & 5 deletions src/perm_group.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ else
mutable struct PermGroup{P<:AbstractPermutation,T<:AbstractTransversal} <:
AbstractPermutationGroup
__gens_raw::Vector{P}
@atomic stabchain::StabilizerChain{P,T}
@atomic order::BigInt
stabchain::StabilizerChain{P,T}
order::BigInt

function PermGroup(
T::Type{<:AbstractTransversal},
Expand Down Expand Up @@ -120,11 +120,11 @@ The subsequent calls just return the cached data structure.
end
else
function StabilizerChain(G::PermGroup{P,T}) where {P,T}
if !isdefined(G, :stabchain, :sequentially_consistent)
if !isdefined(G, :stabchain)
stabchain = schreier_sims(T, __gens_raw(G))
# this may take some time, so let's check again
if !isdefined(G, :stabchain, :sequentially_consistent)
@atomic G.stabchain = stabchain
if !isdefined(G, :stabchain)
G.stabchain = stabchain
end
end
return G.stabchain
Expand Down