Use CacheDictType for caching GF(p)

[sumiya11]

Builds upon #1659 to provide thread-safety by default.

My tentative search shows 1 place with the same issue potentially:

AbstractAlgebra.jl/src/generic/Misc/Localization.jl

Line 39 in 17e182a

LocDict[parent(prime), prime, comp] = r

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.75%. Comparing base (17e182a) to head (f11bb81).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1660      +/-   ##
==========================================
- Coverage   86.75%   86.75%   -0.01%     
==========================================
  Files         115      115              
  Lines       29668    29664       -4     
==========================================
- Hits        25739    25735       -4     
  Misses       3929     3929              

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

[sumiya11]

BTW, regarding localization, there is some strange error, not sure if this warrants an issue or not:

julia> total_ring_of_fractions(ZZ)
ERROR: TypeError: in TotFracRing, in T, expected T<:RingElem, got Type{BigInt}
Stacktrace:
 [1] total_ring_of_fractions(R::AbstractAlgebra.Integers{BigInt}; cached::Bool)
   @ AbstractAlgebra.Generic C:\data\projects\aaa\AbstractAlgebra.jl\src\generic\TotalFraction.jl:617
 [2] total_ring_of_fractions(R::AbstractAlgebra.Integers{BigInt}; cached::Bool)
   @ AbstractAlgebra C:\data\projects\aaa\AbstractAlgebra.jl\src\TotalFraction.jl:25
 [3] total_ring_of_fractions(R::AbstractAlgebra.Integers{BigInt})
   @ AbstractAlgebra C:\data\projects\aaa\AbstractAlgebra.jl\src\TotalFraction.jl:24
 [4] top-level scope
   @ REPL[47]:1

[thofma]

Lets ignore it for now.

[thofma]

Btw, did you test that this makes it thread-safe with cached = true?

[sumiya11]

Yes, it worked fine with @threads.

[thofma]

Strange, it should be broken too. I would suggest to always do cache = false. At some point we will probably make this the default.

[sumiya11]

Strange, it should be broken too. I would suggest to always do cache = false. At some point we will probably make this the default.

Do you mean that using CacheDictType is not correct with cached=true in a multi-threaded setting ?

[thofma]

It might work, but there is no guarantee for it being threadsafe. It might be even worse, because it does not throw an error like a normal dict. It just gets corrupted silently.

[sumiya11]

Sounds scary.

I think I saw that CacheDictType writes/reads using a lock.
Is it not enough for safety here because of some subtle issue ?

[thofma]

Scary? We are talking about multithreading, so everything is scary and hard to get right.

Basic data structures are not and probably never will be threadsafe—it introduces way too much overhead when you don’t need thread safety. If you want to safely share a dict between threads, you can use a lock, or better still, take a page out of Go’s playbook and have a single task which manages the dict and let other tasks send it messages to tell it to update the dict instead.
(https://discourse.julialang.org/t/can-dicts-be-threadsafe/27172/6)

Anyway, it seems that we are indeed using the version with the lock. The purpose of the lock was to make it play nice with the GC. I don't know if this also makes it threadsafe.