JuliaLinearAlgebra / Octavian.jl

Multi-threaded BLAS-like library that provides pure Julia matrix multiplication
https://julialinearalgebra.github.io/Octavian.jl/stable/
Other
232 stars 18 forks source link

Combination of `@time` and `@testset` breaks `getindex` for `PointerMatrix` #9

Closed DilumAluthge closed 3 years ago

DilumAluthge commented 3 years ago

Combination of @time and @testset breaks getindex for PointerMatrix.

(This is on Julia master.)

julia> import Octavian

julia> import Test

julia> Test.@testset "PointerMatrix" begin
           mem = Octavian.L2Buffer(Float64);
           ptr = Base.unsafe_convert(Ptr{Float64}, mem)
           block = Octavian.PointerMatrix(ptr, (10, 20))
           Test.@test Base.unsafe_convert(Ptr{Float64}, block) == pointer(block.p)
           block[1] = 2.3
           Test.@test block[1] ≈ 2.3
           block[4, 5] = 67.89
           Test.@test block[4, 5] ≈ 67.89
       end
Test Summary: | Pass  Total
PointerMatrix |    3      3
Test.DefaultTestSet("PointerMatrix", Any[], 3, false, false)

julia> @time Test.@testset "PointerMatrix" begin
           mem = Octavian.L2Buffer(Float64);
           ptr = Base.unsafe_convert(Ptr{Float64}, mem)
           block = Octavian.PointerMatrix(ptr, (10, 20))
           Test.@test Base.unsafe_convert(Ptr{Float64}, block) == pointer(block.p)
           block[1] = 2.3
           Test.@test block[1] ≈ 2.3
           block[4, 5] = 67.89
           Test.@test block[4, 5] ≈ 67.89
       end
PointerMatrix: Test Failed at REPL[4]:7
  Expression: block[1] ≈ 2.3
   Evaluated: 6.953123393594e-310 ≈ 2.3
Stacktrace:
 [1] macro expansion
   @ ./REPL[4]:7 [inlined]
 [2] macro expansion
   @ ~/dev/repos/JuliaLang/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1148 [inlined]
 [3] macro expansion
   @ ./REPL[4]:2 [inlined]
 [4] top-level scope
   @ ./timing.jl:203 [inlined]
 [5] top-level scope
   @ ./REPL[4]:0
Test Summary: | Pass  Fail  Total
PointerMatrix |    2     1      3
ERROR: Some tests did not pass: 2 passed, 1 failed, 0 errored, 0 broken.

julia> @time begin
           mem = Octavian.L2Buffer(Float64);
           ptr = Base.unsafe_convert(Ptr{Float64}, mem)
           block = Octavian.PointerMatrix(ptr, (10, 20))
           Test.@test Base.unsafe_convert(Ptr{Float64}, block) == pointer(block.p)
           block[1] = 2.3
           Test.@test block[1] ≈ 2.3
           block[4, 5] = 67.89
           Test.@test block[4, 5] ≈ 67.89
       end
  0.000070 seconds (33 allocations: 257.672 KiB)
Test Passed
chriselrod commented 3 years ago

Needs GC.@preserve on the L2Buffer. For example, inside matmul!.

I'd also test for exact equality. Something like

@time Test.@testset "PointerMatrix" begin
           mem = Octavian.L2Buffer(Float64);
           ptr = Base.unsafe_convert(Ptr{Float64}, mem)
           block = Octavian.PointerMatrix(ptr, (10, 20))
           Test.@test Base.unsafe_convert(Ptr{Float64}, block) == pointer(block.p)
           GC.@preserve mem begin
               block[1] = 2.3
               Test.@test block[1] == 2.3
               block[4, 5] = 67.89
               Test.@test block[4, 5] == 67.89
           end
       end

The point of this is that while PointerMatrixes do get passed to non-inlined functions, mem never does in matmul!. Therefore, mem gets stack allocated. But we need the GC.@preserve to protect it for as long as we're using it.

DilumAluthge commented 3 years ago

Perfect!