Store and load each element of every tuple individually

[mmiller-max]

As per this discussion, it is beneficial to store the element of every tuple individually, regardless of isbits or not.

This currently fails one test:

https://github.com/JuliaSIMD/ThreadingUtilities.jl/blob/445f957e7fbaab582dcd65a759b8dd7e4c9996b1/test/internals.jl#L6-L8

Because the array is no longer stored within the tuple and the pointer type is invalid:

julia> ThreadingUtilities.store!(pointer(x), [1.0,2.0], 0)
ERROR: pointerset: invalid pointer type

Do you expect to be able to store an array directly like this, or should StrideArrays really be used?

[chriselrod]

Ah, that may be why I stopped doing that. We could try the Reference trick from StrideArraysCore, moving that code here for AbstractArray{T} where !isbitstype(T).

[mmiller-max]

So just to confirm, I'll move this code from StrideArraysCore to ThreadingUtilities:

# `Reference`s will automatically be unpacked.
mutable struct Reference{T}
    data::T
end

@inline dereference(r::Reference) = getfield(r, :data)
@inline dereference(r) = r
@inline function ThreadingUtilities.load(p::Ptr{UInt}, ::Type{Reference{T}}, i) where {T}
    i, ref = ThreadingUtilities.load(p, Ptr{Reference{T}}, i)
    i, getfield(Base.unsafe_pointer_to_objref(ref)::Reference{T}, :data)::T
    # i, getfield(unsafe_load(ref)::Reference{T}, :data)::T
end
@inline function ThreadingUtilities.store!(p::Ptr{UInt}, r::Reference, i)
    ThreadingUtilities.store!(p + i, reinterpret(UInt, pointer_from_objref(r)))
    i + sizeof(UInt)
end

And add the following to ThreadingUtilities:

@inline store!(p::Ptr{UInt}, arr::AbstractArray{T}, i) where {T} = store!(p, Reference(arr), i)
@inline load(p::Ptr{UInt}, ::Type{T}, i) where {T <: AbstractArray} = load(p, Reference{T}, i)

Couple of questions:

1. Why do we need to check !isbitstype(T) for the element type of an array? In the test above this would be true for [1.0, 2.0], and it looks like the behaviour is the same for ["as","As"]
2. load above seems to be crashing and I can't work out why. Is it because Reference(arr) is gc'd?
3. There is a similar issue with strings (sizeof(String) fails), is the same Reference approach (if the above can work) OK?
4. Are there any other types, or is is bits or isbitstype actually a better way of approaching this?

More details on point 2, this works:

x = zeros(UInt, 100)
GC.@preserve x begin
    ref = Reference([1.0,2.0])
    ThreadingUtilities.store!(pointer(x), ref, 0)
    ThreadingUtilities.load(pointer(x), typeof(ref), 0)
end

But this doesn't:

x = zeros(UInt, 100)
GC.@preserve x begin
    arr = [1.0,2.0]
    ThreadingUtilities.store!(pointer(x), arr, 0)
    ThreadingUtilities.load(pointer(x), typeof(arr), 0)
end

signal (11): Segmentation fault: 11
in expression starting at none:0
gc_mark_loop at /Users/julia/buildbot/worker/package_macos64/build/src/gc.c:2397

[chriselrod]

Sorry, I led you astray earlier/forgot how these packages were setup.

All the actual loading/storing should be done through StrideArraysCore.object_and_preserve, which should be changed to iterate over tuples and store them piece by piece, and assemble the type in that manner as well. It should be the one wrapping things in Reference, and also returning that Reference to be GC.@presereved.

All the References need to be GC.@preserved, so ThreadingUtilities isn't the right place to do that.

[chriselrod]

But CheapThreads.jl should already be doing this: that's exactly what add_var! is supposed to be doing. What's going wrong?

It's meant to generate code that iteratively breaks tuples apart, object_and_preserveing all their contents.

EDIT: Ah, but then it just uses store!/load to do the actual load/storing. So we just need to match its preserve behavior in what the load/store! does.

[chriselrod]

Putting together my previous two comments into something more coherent, CheapThreads.jl will basically do this:

x = zeros(UInt, 100)
ref = Reference([1.0,2.0])
GC.@preserve x ref begin
    ThreadingUtilities.store!(pointer(x), ref, 0)
    ThreadingUtilities.load(pointer(x), typeof(ref), 0)
end

Which is like your working example, so taking that approach should be fine. You shouldn't define store! methods that automatically wrap. Wrapping is object_and_preserve's job. load methods then automatically unwrap, because they assume it was a wrapper added by object_and_preserve, and that the actual object you meant to be loading was what it was wrapping.

[mmiller-max]

Ah OK, that makes sense. So I think we just need to change the tests that are failing here then? That test is now really testing StrideArraysCore functionality by storing unpacked tuple fields that need to be referenced.

I think just removing this test will do:

https://github.com/JuliaSIMD/ThreadingUtilities.jl/blob/445f957e7fbaab582dcd65a759b8dd7e4c9996b1/test/internals.jl#L6-L8

The tuple functionality and storing of 1.0 and 3 are tested by the following test, and "hello world" and [1,2,3,4] need to be referenced and that functionality is actually tested in StrideArraysCore here

https://github.com/JuliaSIMD/StrideArraysCore.jl/blob/fcb1c559b881f5553fa50c8289fc12ecc4e00d62/test/runtests.jl#L123-L128

[chriselrod]

The tuple functionality and storing of 1.0 and 3 are tested by the following test

Okay, you can take that out so long as coverage doesn't change.

[codecov[bot]]

Codecov Report

Merging #23 (dccedc7) into master (445f957) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #23   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          155       156    +1     
=========================================
+ Hits           155       156    +1     

Impacted Files	Coverage Δ
src/utils.jl	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 445f957...dccedc7. Read the comment docs.

[chriselrod]

Line 27 in utils.jl isn't covered:

@inline store!(p::Ptr{UInt}, x::T) where {T} = (unsafe_store!(Base.unsafe_convert(Ptr{T}, p), x); nothing)

This is a generic fallback store function.

[mmiller-max]

OK have added a NamedTuple for the general fallback store case

[chriselrod]

Great, thanks!