Open clouds56 opened 5 years ago
Could we use GC.@preserve
here?
Yes, this use of savedArgs looks likely incorrect.
If it's possible to overload cconvert
for the types which are being passed here, that should be the preferred solution. ccall
already has builtin support for GC rooting its arguments after cconvert
, but before unsafe_convert
.
If for some reason the cconvert
mechanism won't work here, you can use GC.@preserve
.
@aviks was telling me that this code was written before cconvert
existed. We may be able to remove it enitrely and let the ccall
and cconvert
handle it as @c42f suggests.
So here is the commit that added savedArgs
, about 5 years ago: https://github.com/JuliaInterop/JavaCall.jl/commit/6993eb6c834a3358453dd96e6cefe0e94617eaf4 . cconvert
was either very new, or just about to be released. GC.@preserve
certainly wasnt around.
This change was done deliberately, and did fix some reported segfaults, so it did help. And we've not had any reports of JavaCall segfaults in years (not that this is foolproof evidence, but we used to get regular crash reports earlier, but this fix, and another around casting, made all of that go away)
Still, I can sorta see why this might be problematic, and cconvert
might the correct option. But the argument conversions are a bit wierd, and I'm not entirely sure fi cconvert
works in all cases -- if only because I havent had the chance to think about it deeply. GC.@preserve
ing savedArgs
might be a simpler, but uglier, fix
This change was done deliberately, and did fix some reported segfaults, so it did help.
Absolutely, it's very likely that this fixed the crashes in practice!
But if so, this is an implementation detail of the Julia runtime. In the future, it's very likely that Julia codegen will aggressively elide roots more and more often, and these crashes will emerge again.
I don't think it much matters whether you use the ccall
builtin rooting, or GC.@preserve
: choose whichever you think is easier to understand in the context of the JavaCall implementation.
Note also that cconvert
itself doesn't cause any rooting, or really have anything to do with the GC. It's the compilation of ccall
itself which is special: ccall
roots arguments passed to it as they come out of cconvert
, but before they go into unsafe_convert
. If you examine the macro expansion of the new @ccall
macro in 1.5 you will more clearly see how the conversion pipeline works, and which things are rooted:
julia> @macroexpand @ccall foo(x::A, y::B)::C
quote
var"#15#arg1root" = Base.cconvert(A, x)
var"#16#arg1" = Base.unsafe_convert(A, var"#15#arg1root")
var"#17#arg2root" = Base.cconvert(B, y)
var"#18#arg2" = Base.unsafe_convert(B, var"#17#arg2root")
$(Expr(:foreigncall,
:(:foo),
:C,
:(Core.svec(A, B)),
0,
:(:ccall),
Symbol("#16#arg1"),
Symbol("#18#arg2"),
Symbol("#15#arg1root"),
Symbol("#17#arg2root")
))
end
The things named arg$(n)root
are rooted during the foreigncall
(foreigncall is a new internal name for ccall, more or less).
Now JavaCall tries to use
saved_args
to keep reference of JavaObject while passarg.ptr
into JNI FFI. https://github.com/JuliaInterop/JavaCall.jl/blob/d6c3678d3a93fae903fb3ab1cb1ea7ea1fc91f92/src/core.jl#L244-L248But
saved_args
is not referenced after it created, it might be garbage collected at any time (even beforeccall
). I learnt from thisgc
would only occurs when allocates memory (so current code might work now), but it is hard to trace memory allocation.Is there any better solution that telling gc not to collect
saved_args
?