Fix: atomic ordering

[kalmarek]

thanks for the remark @vtjnash

#45 (comment)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.75%. Comparing base (d06251a) to head (6f9a377).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   97.48%   97.75%   +0.27%     
==========================================
  Files          11       11              
  Lines         635      624      -11     
==========================================
- Hits          619      610       -9     
+ Misses         16       14       -2     

Flag	Coverage Δ
unittests	97.75% <100.00%> (+0.27%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kalmarek]

@vtjnash I'd like to get this correct but I have troubles mapping the concepts of
https://marabos.nl/atomics/memory-ordering.html
to this particular situation.

Is this the correct use to form an acquire-release pair?

module Example
mutable struct Ex{T}
   @atomic val::T
   Ex{T}() where T = new{T}()
end
function val(e::Ex{T}) where T
    if !isdefined(e, :val, :acquire)
       val = compute_val(T)
       # throws ConcurrencyViolationError("invalid atomic ordering")
       # @atomiconce :release e.val = val
       @atomiconce :release :acquire e.val = val 
    end
    return e.val
end
compute_val(::Type{<:Integer}) = 42
end

e = Example.Ex{BigInt}()
Example.val(e)

In particular in code for e.g. copy I have this passage

function Base.copy(p::Perm)
    imgs = copy(p.images)
    q = typeof(p)(imgs; check = false)
    if isdefined(p, :inv, :acquire)
        inv_imgs = copy(p.inv.images)
        q⁻¹ = typeof(p)(inv_imgs; check = false)
        @atomic q⁻¹.inv = q
        @atomiconce :release :acquire q.inv = q⁻¹
    end
    return q
end

Where I want to make sure that the @atomic q⁻¹.inv = q really happens before
@atomiconce :release :acquire q.inv = q⁻¹. But I don't seem to understand how to create such happens-before pair in this example.

What should be the ordering for @atomic q⁻¹.inv = q?
What would be the difference if I replaced all of the orderings with :monotonic?

Any clarifications would be much appreciated!

[vtjnash]

The release creates a happens-before, such that all prior writes will have happened if any other thread acquires that new value by reading (no write can finish after the release, no read can start before the acquire, even on other memory)

[araujoms]

I hope I'm not being obnoxious, but it would great for me of this could get merged; I want to add PermutationGroups as a dependency of my package Ket, but I won't do if it means dropping support for 1.12

I looks like the problem is only on 1.10, perhaps support for that could be dropped? I'm afraid I can't be of technical help, as I don't understand any of this multi-threading stuff.

[kalmarek]

@araujoms This fails on lts since there's no @atomiconce (requires VERSION >= v"1.11.0-DEV.1467").
I don't want to drop the support for julia-lts for this package, but I didn't have time to figure out the proper way of using @atomic instead of @atomiconce on lts.

If you want to move this forward PRs are welcome!

[araujoms]

Why not just add a switch to use the current code for 1.10 and @atomiconce for 1.11+?

[kalmarek]

Can you write the switch you have in mind in code?

Note that macros are evaluated before any code is evaluated, all at parse time.
I quickly run out of ideas, given that I didn't spend much time thinking of it.

[araujoms]

Now I managed to understand why CI is happy on 1.12 but on my machine the tests fail: the failing test is the Schreier-Sims benchmark, which you've disabled running in CI with if !(haskey(ENV, "CI")).

[araujoms]

@oscardssmith @vtjnash I hope I'm not abusing your goodwill, but perhaps would you know what is going wrong? After the fixes the code works fine on 1.11, but still crashes on 1.12 and 1.13.

[vtjnash]

GC error (probable corruption) from CI in this case seems like probably a Julia bug. We'd noticed some issues in the past where @atomic stores were discarding / ignoring the gc write barrier. In theory we'd fixed something like that in JuliaLang/julia#59559 about a month ago, but it may end up necessary to take a closer look at what this does that gets wrong codegen, if you file a bug against JuliaLang/julia repo.

[araujoms]

The crashes have been fixed by JuliaLang/julia#59991, but now there's a test failure: the test schr[δ] == z in the testset "Schreier Vectors and Stabilizers". No idea if that's a real error, in my code I now get the correct result.

[kalmarek]

since the crash happens on nightly only and also was observed in #49 I think it's unrelated;

It will need to be investigated though, something I have very little time for right now...