Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mutable rtpsa #121

Closed
wants to merge 5 commits into from
Closed

mutable rtpsa #121

wants to merge 5 commits into from

Conversation

mattsignorelli
Copy link
Contributor

Change the underlying structs to pure C structs?

@Seelengrab
Copy link

Alright, so, my guess for the allocation from unsafe_load is probably wrong, it was most likely for the type tag due to everything happening in global scope. That being said, the data that unsafe_load loads really is the same data as that behind the pointer:

julia> d = Descriptor(2,6)
GTPSA Descriptor
-----------------------
Maximum order:     6
# Variables:       2


julia> tpsa = GTPSA.mad_tpsa_newd(d.desc, GTPSA.MAD_TPSA_SAME) # this is C ctor
Ptr{GTPSA.RTPSA}(0x00005d195b78c898)

julia> rtpsa = unsafe_load(tpsa)
GTPSA.RTPSA(Ptr{Nothing}(0x00005d1959accb28), 0x01, 0x00, 0x06, 0x06, 0, (0x00, 0x0a, 0x8f, 0x5b, 0x19, 0x5d, 0x00, 0x00, 0x40, 0x0b, 0x8f, 0x5b, 0x19, 0x5d, 0x00, 0x00), Ptr{Float64}(0x0000000000000000))

julia> unsafe_load(convert(Ptr{Ptr{Cvoid}}, tpsa))
Ptr{Nothing}(0x00005d1959accb28)

julia> unsafe_load(convert(Ptr{Ptr{Cvoid}}, tpsa)) == rtpsa.d
true

So, something somewhere messes about that shouldn't 🤔

@mattsignorelli
Copy link
Contributor Author

@Seelengrab Thank you for all your help with this!

Even though the data is the same, it seems unsafe_load is actually allocating a new RTPSA:

julia> unsafe_load(tpsa) === unsafe_load(tpsa)
false

So the question really is, does the GC follow the same rules for Ptr{RTPSA} as it would for RTPSA?

@Seelengrab
Copy link

Seelengrab commented Jul 12, 2024

So the question really is, does the GC follow the same rules for Ptr{RTPSA} as it would for RTPSA?

To the language, the two objects are not linked at all. A Ptr is really just the raw address; the runtime doesn't "look into" the pointer. So in that case, the best route forwards for you really is to get GTPSA to use the julia allocator internally. That's a bummer, sorry for giving you false hope 😔 This really ought to be added to the docstring of unsafe_load though..

I know of a similar case in Vulkan.jl, where they have high-level julia-GC managed objects that transparently track & free GPU ressources as necessary. Maybe that could be helpful? You can find some docs about their approach here.

@mattsignorelli
Copy link
Contributor Author

That is unfortunate. I guess one possible solution would be to write the constructors fully in Julia.

I wonder why unsafe_pointer_to_objref segfaults, this seems like a bug

@Seelengrab
Copy link

That is unfortunate. I guess one possible solution would be to write the constructors fully in Julia.

Yes, that ought to work too. You'll still have to keep the low level structs around to convert to & from though when passing to your external library.

I wonder why unsafe_pointer_to_objref segfaults, this seems like a bug

No, that's expected; the Ptr you're passing to unsafe_pointer_to_objref doesn't refer to a julia-allocated object after all and is missing the type tag.

@mattsignorelli
Copy link
Contributor Author

OK so here is the proposed solution: Because the RTPSA and CTPSA constructors are not very complicated, I will write them in pure Julia. jl_malloc, called from Julia, will be used to allocate the coef array. So no changes to the C library are necessary. I then will refactor all functions to work with these types. If there is no reduction in performance, then this solution will be merged into main.

@Seelengrab
Copy link

I hope it works!

@mattsignorelli
Copy link
Contributor Author

mattsignorelli commented Jul 12, 2024

This is not very trivial because coef is a flexible array member in the C library

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants