JuliaApproximation / FastTransforms.jl

:rocket: Julia package for orthogonal polynomial transforms :snowboarder:
Other
259 stars 27 forks source link

change Ptr to Ref for Julia variables passed as ccall arguments #214

Closed jishnub closed 1 year ago

jishnub commented 1 year ago

From what I understand, memory that is managed by Julia should use a Ref instead of a Ptr, to ensure that the object is not freed while the ccall is in progress. Ptr is to be used for memory that is managed by C.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 79.48% and no project coverage change.

Comparison is base (d7be08c) 81.72% compared to head (9aa83d8) 81.72%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #214 +/- ## ======================================= Coverage 81.72% 81.72% ======================================= Files 15 15 Lines 2534 2534 ======================================= Hits 2071 2071 Misses 463 463 ``` | [Impacted Files](https://codecov.io/gh/JuliaApproximation/FastTransforms.jl/pull/214?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation) | Coverage Δ | | |---|---|---| | [src/libfasttransforms.jl](https://codecov.io/gh/JuliaApproximation/FastTransforms.jl/pull/214?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation#diff-c3JjL2xpYmZhc3R0cmFuc2Zvcm1zLmps) | `77.61% <79.48%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaApproximation)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

MikaelSlevinsky commented 1 year ago

Checking the docs for ccall, they say

Each argvalue to the ccall will be converted to the corresponding argtype, by automatic insertion of calls to unsafe_convert(argtype, cconvert(argtype, argvalue)). (See also the documentation for unsafe_convert and cconvert for further details.) In most cases, this simply results in a call to convert(argtype, argvalue).

So when I type

julia> X = rand(2,2)
2×2 Matrix{Float64}:
 0.523683  0.123465
 0.018049  0.0141381

julia> Base.unsafe_convert(Ptr{Float64}, Base.cconvert(Ptr{Float64}, X))
Ptr{Float64} @0x00000001085a4b20

julia> Base.unsafe_convert(Ref{Float64}, Base.cconvert(Ref{Float64}, X))
Ptr{Float64} @0x00000001085a4b20

I see the same return values.

Notice that FFTW's ccalls use a lot of Ptr{Floatxx} still, so I don't understand how this could possibly be an issue. Please advise otherwise. https://github.com/JuliaMath/FFTW.jl/blob/master/src/fft.jl

jishnub commented 1 year ago

The returned values are identical, but the difference is that the Julia GC doesn't free memory that has a Refto it, but it can free memory that has a Ptr to it, so a Ptr isn't guaranteed to point to a valid memory address. A workaround is to use GC.@preserve if a Ptr is passed.

I was referring to this part

For C code accepting pointers, Ref{T} should generally be used for the types of input arguments, allowing the use of pointers to memory managed by either Julia or C through the implicit call to Base.cconvert. In contrast, pointers returned by the C function called should be declared to be of output type Ptr{T}, reflecting that the memory pointed to is managed by C only.

This difference may not matter in many cases, e.g. if the arrays are used in the outer scope and thus protected from GC. However, perhaps it's better to be on the safer side?

jishnub commented 1 year ago

I'm closing this for now, until I receive further clarification on this issue