Skip to content

Fix for Distributed use of WrappedRNG #31

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

Merged
andreasnoack merged 4 commits into from
Jan 23, 2025
Merged

Fix for Distributed use of WrappedRNG #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
195c1fe
Use reinterpret instead of unsafe_wrap for WrappedRNG field vals
GregPlowman Jan 16, 2025
c4a224c
Test Distributed smallcrushJulia
GregPlowman Jan 16, 2025
b29b024
using Distributed in runtests.jl
GregPlowman Jan 16, 2025
88c3567
Ensure Xoshiro is defined before use
GregPlowman Jan 16, 2025
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
4 changes: 2 additions & 2 deletions src/RNGTest.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ module RNGTest
rng::RNG
cache::Vector{T}
fillarray::Bool
vals::Vector{UInt32}
vals::Union{Vector{UInt32}, Base.ReinterpretArray{UInt32, 1, T, Vector{T}, false}}
Copy link
Collaborator

@andreasnoack andreasnoack Jan 21, 2025

Choose a reason for hiding this comment

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

Can't this be made a ReinterpretArray unconditionally?

Copy link
Contributor Author

@GregPlowman GregPlowman Jan 21, 2025

Choose a reason for hiding this comment

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

I was hoping so too, but when the cache itself is a Vector{UInt32}, reinterpreting it just returns the cache.

cache = zeros(UInt32, 100)
reinterpret(UInt32, cache) === cache

I'm not sure how to make this cleaner.

idx::Int
end

Expand All @@ -45,7 +45,7 @@ module RNGTest
end
cache = Vector{T}(undef, cache_size)
fillcache(WrappedRNG{T, RNG}(rng, cache, fillarray,
unsafe_wrap(Array, convert(Ptr{UInt32}, pointer(cache)), sizeof(cache)÷sizeof(UInt32)),
Copy link
Collaborator

@andreasnoack andreasnoack Jan 20, 2025

Choose a reason for hiding this comment

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

It's not clear to me why this pointer would become invalid. It is used right away so I don't see where a pointer would be transferred between processes. Is there something I'm missing here?

Copy link
Contributor Author

@GregPlowman GregPlowman Jan 21, 2025

Choose a reason for hiding this comment

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

The problem arises when the cache needs to be refilled. Presumably, the intention is for vals to be a reinterpreted view of cache with the same underlying data (so when cache is updated, vals also refers to this updated cache).

I don't have a deep understanding of Julia internals, but here's my best guess of what's happening.

I suspect using unsafe_wrap with pointers creates a "normal" Vector but with its backing memory assigned to cache. This works locally. However, when WrappedRNG is serialized to remote workers, I suspect vals is copied normally and the "view" relationship to cache is lost. At this point vals is a reinterpreted copy (not view) of cache and the problem won't immediately occur. But when the cache is refilled, vals will be unchanged and not represent the data in cache.

The fix here is using reinterpret(UInt32, cache) which creates a special type. When that type is serialized to remote workers, the reinterpreted view into cache is preserved, rather than just copying the underlying data.

An alternative fix might be to reassign vals using unsafe_wrap in fillcache. I'm happy to try that fix if you prefer. However I think the explicit use of reinterpret (and the type ReinterpretArray) avoids Julia internals, is more efficient and better documents the semantic intention.

Here's some test code that shows the problem.

using Distributed
addprocs(1)

@everywhere begin
    using Random
    using RNGTest

    function test(wrng::RNGTest.WrappedRNG)
        t1 = reinterpret(UInt32, wrng.cache) == wrng.vals
        v1 = copy(wrng.vals)
        RNGTest.fillcache(wrng)
        t2 = reinterpret(UInt32, wrng.cache) == wrng.vals
        t3 = wrng.vals != v1
        return t1, t2, t3
    end
end

rng = Xoshiro()
wrng = RNGTest.wrap(rng, UInt64)

test(wrng)
remotecall_fetch(test, 2, wrng)

Copy link
Collaborator

@andreasnoack andreasnoack Jan 21, 2025

Choose a reason for hiding this comment

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

That makes sense. So the vals array should always alias the cache array but that aliasing breaks when moving between processes. Indeed, using reinterpret seems like the right solution.

reinterpret(UInt32, cache),
0)) # 0 is a dummy value, which will be set correctly by fillcache
end

Expand Down
16 changes: 16 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using Test
using RNGTest
using Random
using Distributed

f = rand
pval = 0.001
Expand Down Expand Up @@ -160,3 +161,18 @@ end
RNGTest.smallcrushTestU01(rng)
@test all(t -> t > pval, mapreduce(s -> [s...], vcat, RNGTest.smallcrushJulia(rng)))
end

@testset "Distributed smallcrushJulia" begin
pids = addprocs()
@everywhere using RNGTest
for T in (UInt32, UInt64, Float64)
if isdefined(Random, :Xoshiro)
rng = RNGTest.wrap(Xoshiro(), T)
else
rng = RNGTest.wrap(MersenneTwister(), T)
end
results = RNGTest.smallcrushJulia(rng)
@test all(ps -> all(>(pval), ps), results)
end
rmprocs(pids)
end
Loading