search

JuliaInterop / libcxxwrap-julia

C++ library for backing CxxWrap.jl
Other
84 stars 43 forks source link

Add JL_GC_PUSH / JL_GC_POP calls, reduce usage of jl_svec1 #74

Closed fingolfin closed 3 years ago

fingolfin commented 3 years ago

Creating a new type or allocating an array might garbage collect the given type unless we GC push/pop it.

Perhaps I am mistaken, though, and these types are in no danger of being GC'ed; in this case I'd appreciate a hint why that is so, to further my understanding of this codebase. Thanks!

yuyichao commented 3 years ago

It's not needed for any concrete types.

fingolfin commented 3 years ago

@yuyichao OK thanks for clarifying this! I've dropped all push/pop for concrete types now; but there still were a bunch of svec being created on the fly which were not pushed/popped; I eliminate a bunch of them, and add JL_GC_PUSH/POP treatment for the rest.

fingolfin commented 3 years ago

@barche Actually, I don't understand at all how your Travis tests work: I don't see the code which overrides the code in the libcxxwrap_julia_jll.jl artifact with the one produced by compiling this repository. Can you give me a pointer?

rgcv commented 3 years ago

@fingolfin There's an option you can set when running CMake named APPEND_OVERRIDES_TOML. This will add an entry to the artifact overrides toml in your home julia folder, pointing to the artifact built locally.

https://github.com/JuliaInterop/libcxxwrap-julia/blob/2bba0c81ea00d58d3321540a0526098aa9eb3c8b/CMakeLists.txt#L229-L238

The option is used in the travis config file.

https://github.com/JuliaInterop/libcxxwrap-julia/blob/2bba0c81ea00d58d3321540a0526098aa9eb3c8b/.travis.yml#L22

Hope that helps clear things up.

fingolfin commented 3 years ago

@barche can this be merged? Let me know if you have any concerns or questions. While it doesn't seem to help with the bug I wanted to fix, I think it's still an improvement in that it both fixes GC correctness issues and avoids some (tiny) allocation which always is helpful.

barche commented 3 years ago

Thanks for this, I only renamed apply_type1.