Fix: atomic ordering

.github/workflows/ci.yml

@@ -15,7 +15,7 @@ jobs:
       fail-fast: false
       matrix:
         version:
-          - '1.6'
+          - 'lts'
           - '1'
           - 'nightly'
         os:

.github/workflows/format.yml

@@ -14,11 +14,11 @@ jobs:
     steps:
       - uses: julia-actions/setup-julia@latest
         with:
-          version: '^1.6'
+          version: 'lts'
       - uses: actions/checkout@v2
       - name: Install JuliaFormatter and format
         run: |
-          julia  -e 'using Pkg; Pkg.add(PackageSpec(name="JuliaFormatter", version="1.0.45"))'
+          julia  -e 'using Pkg; Pkg.add(PackageSpec(name="JuliaFormatter", version="2"))'
           julia  -e 'using JuliaFormatter; format(["./src", "./test"], verbose=true)'
       - name: Format check
         run: |

Project.toml

@@ -13,7 +13,7 @@ Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
 AbstractPermutations = "0.3"
 GroupsCore = "0.5"
 PrecompileTools = "1"
-julia = "1.6"
+julia = "1.10"
 
 [extras]
 BenchmarkTools = "6e4b80f9-dd63-53aa-95a3-0cdb28fa8baf"

src/Perms/perm_images.jl

@@ -1,35 +1,17 @@
-@static if VERSION < v"1.7"
-    mutable struct Perm{T<:Integer} <: AP.AbstractPermutation
-        images::Vector{T}
-        inv::Perm{T}
-        cycles::AP.CycleDecomposition{T}
+mutable struct Perm{T<:Integer} <: AP.AbstractPermutation
+    images::Vector{T}
+    @atomic inv::Perm{T}
+    @atomic cycles::AP.CycleDecomposition{T}
 
-        function Perm{T}(images::Vector{T}; check::Bool = true) where {T}
-            if check && !isperm(images)
-                throw(ArgumentError("Provided images are not permutation!"))
-            end
-            deg = __degree(images)
-            resize!(images, deg)
-            return new{T}(images)
-        end
-    end
-else
-    mutable struct Perm{T<:Integer} <: AP.AbstractPermutation
-        images::Vector{T}
-        @atomic inv::Perm{T}
-        @atomic cycles::AP.CycleDecomposition{T}
-
-        function Perm{T}(images::Vector{T}; check::Bool = true) where {T}
-            if check && !isperm(images)
-                throw(ArgumentError("Provided images are not permutation!"))
-            end
-            li = lastindex(images)
-            deg =
-                iszero(li) ? li :
-                @inbounds images[li] ≠ li ? li : __degree(images)
-            resize!(images, deg)
-            return new{T}(images)
+    function Perm{T}(images::Vector{T}; check::Bool = true) where {T}
+        if check && !isperm(images)
+            throw(ArgumentError("Provided images are not permutation!"))
         end
+        li = lastindex(images)
+        deg =
+            iszero(li) ? li : @inbounds images[li] ≠ li ? li : __degree(images)
+        resize!(images, deg)
+        return new{T}(images)
     end
 end
 
@@ -59,69 +41,77 @@ AP.inttype(::Type{Perm{T}}) where {T} = T
 AP.inttype(::Type{Perm}) = UInt16
 AP.__unsafe_image(n::Integer, σ::Perm) = oftype(n, @inbounds σ.images[n])
 
-@static if VERSION < v"1.7"
+@static if VERSION < v"1.11"
     function Base.copy(p::Perm)
         imgs = copy(p.images)
         q = typeof(p)(imgs; check = false)
-        if isdefined(p, :inv)
-            inv_imgs = copy(p.inv.images)
+        if isdefined(p, :inv, :sequentially_consistent)
+            inv_imgs = copy(@atomic(p.inv).images)
             q⁻¹ = typeof(p)(inv_imgs; check = false)
-            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)
-            σ⁻¹ = typeof(σ)(invperm(σ.images); check = false)
-            σ.inv = σ⁻¹
-            σ⁻¹.inv = σ
+        if !isdefined(σ, :inv, :sequentially_consistent)
+            if isone(σ)
+                @atomic σ.inv = σ
+            else
+                σ⁻¹ = typeof(σ)(invperm(σ.images); check = false)
+                # we don't want to end up with two copies of inverse σ floating around
+                if !isdefined(σ, :inv, :sequentially_consistent)
+                    @atomic σ.inv = σ⁻¹
+                    @atomic σ⁻¹.inv = σ
+                end
+            end
         end
         return σ.inv
     end
 
     function AP.cycles(σ::Perm)
-        if !isdefined(σ, :cycles)
+        if !isdefined(σ, :cycles, :sequentially_consistent)
             cdec = AP.CycleDecomposition(σ)
-            σ.cycles = cdec
+            # we can afford producing more than one cycle decomposition
+            @atomic σ.cycles = cdec
         end
         return σ.cycles
     end
 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, :acquire)
+            inv_imgs = copy(p.inv.images)
             q⁻¹ = typeof(p)(inv_imgs; check = false)
-            @atomic q.inv = q⁻¹
-            @atomic q⁻¹.inv = q
+            @atomic :release q⁻¹.inv = q
+            @atomiconce :release :acquire q.inv = q⁻¹
         end
         return q
     end
 
     function Base.inv(σ::Perm)
-        if !isdefined(σ, :inv, :sequentially_consistent)
+        if !isdefined(σ, :inv, :acquire)
             if isone(σ)
-                @atomic σ.inv = σ
+                @atomiconce :release :acquire σ.inv = σ
             else
                 σ⁻¹ = typeof(σ)(invperm(σ.images); check = false)
-                # we don't want to end up with two copies of inverse σ floating around
-                if !isdefined(σ, :inv, :sequentially_consistent)
-                    @atomic σ.inv = σ⁻¹
-                    @atomic σ⁻¹.inv = σ
-                end
+                # this order is important:
+                # fuly initialize the "local" inverse first and only then
+                # update σ to make the local inverse visible globally
+                @atomic :release σ⁻¹.inv = σ
+                @atomiconce :release :acquire σ.inv = σ⁻¹
             end
         end
         return σ.inv
     end
 
     function AP.cycles(σ::Perm)
-        if !isdefined(σ, :cycles, :sequentially_consistent)
+        if !isdefined(σ, :cycles, :acquire)
             cdec = AP.CycleDecomposition(σ)
             # we can afford producing more than one cycle decomposition
-            @atomic σ.cycles = cdec
+            @atomiconce :release :acquire σ.cycles = cdec
         end
         return σ.cycles
     end

src/group_interface.jl

@@ -1,23 +1,25 @@
 # Group Interface
 
 Base.one(G::PermGroup{P}) where {P} = Permutation(one(P), G)
-@static if VERSION < v"1.7"
+
+@static if VERSION < v"1.11"
     function GroupsCore.order(::Type{T}, G::AbstractPermutationGroup) where {T}
-        if !isdefined(G, :order)
+        if !isdefined(G, :order, :sequentially_consistent)
             ord = order(StabilizerChain(G))
-            G.order = ord
+            @atomic G.order = ord
         end
         return convert(T, G.order)
     end
 else
     function GroupsCore.order(::Type{T}, G::AbstractPermutationGroup) where {T}
-        if !isdefined(G, :order, :sequentially_consistent)
-            ord = order(StabilizerChain(G))
-            @atomic G.order = ord
+        if !isdefined(G, :order, :acquire)
+            ord = order(BigInt, StabilizerChain(G))
+            @atomiconce :release :acquire G.order = ord
         end
         return convert(T, G.order)
     end
 end
+
 GroupsCore.gens(G::PermGroup) = Permutation.(G.__gens_raw, Ref(G))
 
 function Random.Sampler(

src/perm_group.jl

@@ -5,39 +5,20 @@ Permutation group generated by `gens`.
 `PermGroup`s are by definition sub-groups of the full symmetric group. Order
 and stabilizer chain are computed (and cached) _when needed_.
 """
-@static if VERSION < v"1.7"
-    mutable struct PermGroup{P<:AbstractPermutation,T<:AbstractTransversal} <:
-                   AbstractPermutationGroup
-        __gens_raw::Vector{P}
-        stabchain::StabilizerChain{P,T}
-        order::BigInt
-
-        function PermGroup(
-            T::Type{<:AbstractTransversal},
-            gens::AbstractVector{<:AbstractPermutation},
-        )
-            @assert !isempty(gens) "groups need to have at least one generator"
-            gens_raw = [AP.perm(s) for s in gens]
-            Tr = __schreier_sims_transversal(T, eltype(gens_raw))
-            return new{eltype(gens_raw),Tr}(gens_raw)
-        end
-    end
-else
-    mutable struct PermGroup{P<:AbstractPermutation,T<:AbstractTransversal} <:
-                   AbstractPermutationGroup
-        __gens_raw::Vector{P}
-        @atomic stabchain::StabilizerChain{P,T}
-        @atomic order::BigInt
-
-        function PermGroup(
-            T::Type{<:AbstractTransversal},
-            gens::AbstractVector{<:AbstractPermutation},
-        )
-            @assert !isempty(gens) "groups need to have at least one generator"
-            gens_raw = [AP.perm(s) for s in gens]
-            Tr = __schreier_sims_transversal(T, eltype(gens_raw))
-            return new{eltype(gens_raw),Tr}(gens_raw)
-        end
+mutable struct PermGroup{P<:AbstractPermutation,T<:AbstractTransversal} <:
+               AbstractPermutationGroup
+    __gens_raw::Vector{P}
+    @atomic stabchain::StabilizerChain{P,T}
+    @atomic order::BigInt
+
+    function PermGroup(
+        T::Type{<:AbstractTransversal},
+        gens::AbstractVector{<:AbstractPermutation},
+    )
+        @assert !isempty(gens) "groups need to have at least one generator"
+        gens_raw = [AP.perm(s) for s in gens]
+        Tr = __schreier_sims_transversal(T, eltype(gens_raw))
+        return new{eltype(gens_raw),Tr}(gens_raw)
     end
 end
 
@@ -107,25 +88,22 @@ The first call on a particular group `G` will construct the chain from `gens(G)`
 and complete it by the deterministic Schreier-Sims algorithm.
 The subsequent calls just return the cached data structure.
 """
-@static if VERSION < v"1.7"
+@static if VERSION < v"1.11"
     function StabilizerChain(G::PermGroup{P,T}) where {P,T}
-        if !isdefined(G, :stabchain)
+        if !isdefined(G, :stabchain, :sequentially_consistent)
             stabchain = schreier_sims(T, __gens_raw(G))
             # this may take some time, so let's check again
-            if !isdefined(G, :stabchain)
-                G.stabchain = stabchain
+            if !isdefined(G, :stabchain, :sequentially_consistent)
+                @atomic G.stabchain = stabchain
             end
         end
         return G.stabchain
     end
 else
     function StabilizerChain(G::PermGroup{P,T}) where {P,T}
-        if !isdefined(G, :stabchain, :sequentially_consistent)
+        if !isdefined(G, :stabchain, :acquire)
             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
-            end
+            @atomiconce :release :acquire G.stabchain = stabchain
         end
         return G.stabchain
     end

src/stabchain.jl

@@ -173,7 +173,7 @@ end
 function Base.iterate(lfs::Leafs{<:AbstractTransversal})
     states = last.(iterate.(lfs.iters))
 
-    partial_prods = map(1:length(lfs.iters)-1) do idx
+    partial_prods = map(1:(length(lfs.iters)-1)) do idx
         tr = lfs.iters[idx]
         return tr[first(tr)]
     end

test/benchmark.jl

@@ -17,14 +17,13 @@ end
 const GENERATORS = Dict(
     "Sym8_7t" => parse.(Perm{UInt16}, ["($(i), $(i+1))" for i in 1:7]),
     "Sym8_2rp" => [perm"(1,5,6,2,4,8)", perm"(1,3,6)(2,5,7,4)(8)"],
-    "cube222" =>
-        Perm.([
-            [1, 9, 3, 11, 5, 13, 7, 15, 2, 10, 4, 12, 6, 14, 8, 16],
-            [1, 2, 3, 4, 9, 10, 11, 12, 5, 6, 7, 8, 13, 14, 15, 16],
-            [1, 2, 5, 6, 3, 4, 7, 8, 9, 10, 13, 14, 11, 12, 15, 16],
-            [16, 8, 14, 6, 12, 4, 10, 2, 15, 7, 13, 5, 11, 3, 9, 1],
-            [3, 11, 1, 9, 7, 15, 5, 13, 4, 12, 2, 10, 8, 16, 6, 14],
-        ]),
+    "cube222" => Perm.([
+        [1, 9, 3, 11, 5, 13, 7, 15, 2, 10, 4, 12, 6, 14, 8, 16],
+        [1, 2, 3, 4, 9, 10, 11, 12, 5, 6, 7, 8, 13, 14, 15, 16],
+        [1, 2, 5, 6, 3, 4, 7, 8, 9, 10, 13, 14, 11, 12, 15, 16],
+        [16, 8, 14, 6, 12, 4, 10, 2, 15, 7, 13, 5, 11, 3, 9, 1],
+        [3, 11, 1, 9, 7, 15, 5, 13, 4, 12, 2, 10, 8, 16, 6, 14],
+    ]),
     "cube333" => [
         perm"( 1, 3, 8, 6)( 2, 5, 7, 4)( 9,33,25,17)(10,34,26,18)(11,35,27,19)",
         perm"( 9,11,16,14)(10,13,15,12)( 1,17,41,40)( 4,20,44,37)( 6,22,46,35)",
@@ -70,6 +69,13 @@ const GENERATORS = Dict(
 )
 
 @testset "GAP Docs examples" begin
+    @testset "SL(4,7)" begin
+        SL_4_7tr = PermGroup(Transversal, GENERATORS["SL(4,7)"])
+        @test order(Int64, SL_4_7tr) == 2317591180800
+        SL_4_7str = PermGroup(SchreierTransversal, GENERATORS["SL(4,7)"])
+        @test order(Int64, SL_4_7str) == 2317591180800
+    end
+
     @testset "Sym(8) iteration" begin
         G = PermGroup(GENERATORS["Sym8_7t"])
         Ktr = PermGroup(Transversal, GENERATORS["Sym8_2rp"])
@@ -96,13 +102,6 @@ const GENERATORS = Dict(
         @test order(Int128, RC3str) == 43252003274489856000
     end
 
-    @testset "SL(4,7)" begin
-        SL_4_7tr = PermGroup(Transversal, GENERATORS["SL(4,7)"])
-        @test order(Int64, SL_4_7tr) == 2317591180800
-        SL_4_7str = PermGroup(SchreierTransversal, GENERATORS["SL(4,7)"])
-        @test order(Int64, SL_4_7str) == 2317591180800
-    end
-
     @testset "DirectProduct example" begin
         Gtr = PermGroup(Transversal, GENERATORS["direct_product"])
         @test order(Int, Gtr) == 192480
@@ -169,12 +168,6 @@ if !(haskey(ENV, "CI"))
         @btime test_perf($Gstr)
     end
 end
-@testset "SL(4,7)" begin
-    SL_4_7tr = PermGroup(Transversal, GENERATORS["SL(4,7)"])
-    @test order(Int64, SL_4_7tr) == 2317591180800
-    SL_4_7str = PermGroup(SchreierTransversal, GENERATORS["SL(4,7)"])
-    @test order(Int64, SL_4_7str) == 2317591180800
-end
 
 #=
 Julia Version 1.10.2

test/schreier_sims.jl

@@ -71,14 +71,13 @@
     @test PG.gens(sc2) == PG.gens(sc)
 
     @testset "perm from base images" begin
-        cube222 =
-            Perm.([
-                [1, 9, 3, 11, 5, 13, 7, 15, 2, 10, 4, 12, 6, 14, 8, 16],
-                [1, 2, 3, 4, 9, 10, 11, 12, 5, 6, 7, 8, 13, 14, 15, 16],
-                [1, 2, 5, 6, 3, 4, 7, 8, 9, 10, 13, 14, 11, 12, 15, 16],
-                [16, 8, 14, 6, 12, 4, 10, 2, 15, 7, 13, 5, 11, 3, 9, 1],
-                [3, 11, 1, 9, 7, 15, 5, 13, 4, 12, 2, 10, 8, 16, 6, 14],
-            ])
+        cube222 = Perm.([
+            [1, 9, 3, 11, 5, 13, 7, 15, 2, 10, 4, 12, 6, 14, 8, 16],
+            [1, 2, 3, 4, 9, 10, 11, 12, 5, 6, 7, 8, 13, 14, 15, 16],
+            [1, 2, 5, 6, 3, 4, 7, 8, 9, 10, 13, 14, 11, 12, 15, 16],
+            [16, 8, 14, 6, 12, 4, 10, 2, 15, 7, 13, 5, 11, 3, 9, 1],
+            [3, 11, 1, 9, 7, 15, 5, 13, 4, 12, 2, 10, 8, 16, 6, 14],
+        ])
         sc = PG.schreier_sims(cube222)
         β = PG.basis(sc)
         @test all(PG.leafs(sc)) do g