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

Proper handling of some callbacks and create some wrapper objects with automatic memory management #132

Merged
merged 14 commits into from
Feb 2, 2024

Conversation

yuyichao
Copy link
Collaborator

@yuyichao yuyichao commented Jan 4, 2024

This only cover a few of the functions (the ones that have macros created for them) and all of their uses. There are still many other callback types and objects that are not covered.

All the new interfaces should be restricted to when it is actually safe to do so. The old interface remains in place for backward compatibility (it may be broken for a few cases if the user have defined their own cconvert/unsafe_convert methods on the GSL pointer type but I think those usecases can be safely ignored.)

The auto generation script has not been updated. There are some changes to the code that can't easily be done automatically though most of those should not be exported function anymore (there's method explicitly defined in GSL module that hides the C module reexport) so that could be fixed reasonably easily.

The implementation for some functions (the "managed" ones, e.g. cheb_init are not fully optimum) and there are some cases where another one allocation could be shaved off. Since these are mainly set-once-use-multiple time allocations I didn't feel like optimizing it while making the code a bit more ugly at the moment....

@lxvm lxvm mentioned this pull request Jan 5, 2024
7 tasks
@lxvm
Copy link
Contributor

lxvm commented Jan 5, 2024

I took a look at GSL.jl's dependents and observed the following usage of types and routines:

Package usage of GSL.jl
  • Theta.jl: sf_gamma_inc
  • RandomMatrices.jl: gsl_permutation
  • PhaseSpaceTools.jl: sf_laguerre_n
  • MultipleScattering.jl: sf_legendre_array
  • MoMKernels.jl: sf_bessel_yl, sf_bessel_j1, sf_bessel_yl_array, sf_bessel_j1_array
  • Luna.jl: hypergeom sf_bessel_zero_Jnu
  • HyperGeoMat.jl: sf_lngamma, sf_lngamma_complex_e
  • Hankel.jl: sf_bessel_zero_J0, sf_bessel_zero_J1, sf_bessel_zero_Jnu
  • FluxReconstruction.jl: sf_legendre_Pl_deriv_array
  • EffectiveWaves.jl: sf_bessel_jl_array
  • DickeModel.jl: sf_laguerre_n, sf_legendre_array_e, sf_hermite_phys_array, sf_legendre_sphPlm,
  • Copulas.jl: sf_debye_1, removed integration_qags, Fix broken kendall tau on GumbelBarnettCopula and InvGaussianCopula lrnv/Copulas.jl#72
  • BivariateCopulas.jl: sf_debye_1

which is almost entirely special functions, so this api wouldn't immediately affect the package ecosystem. I think a great benefit of this work is that wrapping these routines in higher-level interfaces such as Integrals.jl and NonlinearSolve.jl becomes greatly simplified.

I also went through the package to identify as many of the callback types and unsafe interfaces as I could find.

Callback wrapper types

All of these types can have helper macros to convert user callbacks into cfunctions, however we should only add cconvert methods when there are routines that can use a callback safely in a ccall (e.g. integration_cquad, multiroot_fdjacobian, and in the routines mentioned below in "Other types")

callback wrapper types types with pointers to callback notes
gsl_function gsl_min_fminimizer has macro and cconvert
gsl_root_fsolver
gsl_function_vec unused upstream
gsl_function_fdf gsl_root_fdfsolver has macro
gsl_multiroot_function gsl_multiroot_fsolver has macro and cconvert
gsl_multiroot_function_fdf gsl_multiroot_fdfsolver has macro
gsl_monte_function
gsl_movstat_function
gsl_multifit_function gsl_multifit_fsolver
gsl_multifit_function_fdf gsl_multifit_fdfsolver
gsl_multifit_fdfridge
gsl_multifit_nlinear_fdf gsl_multifit_nlinear_trust_state
gsl_multifit_nlinear_workspace
gsl_multilarge_nlinear_fdf gsl_multilarge_nlinear_trust_state
gsl_multilarge_nlinear_workspace
gsl_multimin_function gsl_multimin_fminimizer
gsl_multimin_function_fdf gsl_multimin_fdfminimizer
Types storing callback pointers

In order to be safe, helper routines for these wrappers, i.e. all except for the alloc, need to be restricted to when it is actually safe to do so, as mentioned before.

(it may be broken for a few cases if the user have defined their own cconvert/unsafe_convert methods on the GSL pointer type but I think those usecases can be safely ignored.)

type unsafe routines safe alternative
gsl_min_fminimizer min_fminimizer_alloc GSLMinFMinimizer
min_fminimizer_set
min_fminimizer_set_with_values
gsl_root_fsolver root_fsolver_alloc GSLRootFSolver
root_fsolver_set
gsl_root_fdfsolver root_fdfsolver_alloc GSLRootFDFSolver
root_fdfsolver_set
gsl_multiroot_fsolver multiroot_fsolver_alloc GSLMultirootFSolver
multiroot_fsolver_set
gsl_multiroot_fdfsolver multiroot_fdfsolver_alloc GSLMultirootFDFSolver
multiroot_fdfsolver_set
gsl_multifit_fsolver multifit_fsolver_alloc
multifit_fsolver_set
gsl_multifit_fdfsolver multifit_fdfsolver_alloc
multifit_fdfsolver_set
multifit_fdfsolver_wset
gsl_multifit_fdfridge multifit_fdfridge_alloc
multifit_fdfridge_set
multifit_fdfridge_set2
multifit_fdfridge_set3
multifit_fdfridge_wset
multifit_fdfridge_wset2
multifit_fdfridge_wset3
gsl_multifit_nlinear_trust_state unused
gsl_multifit_nlinear_workspace multifit_nlinear_alloc
multifit_nlinear_init
multifit_nlinear_df
multifit_nlinear_fdfvv
gsl_multilarge_nlinear_trust_state unused
gsl_multilarge_nlinear_workspace multilarge_nlinear_alloc
multilarge_nlinear_init
multilarge_nlinear_df
multilarge_nlinear_fdfvv
gsl_multimin_fminimizer multimin_fminimizer_alloc
multimin_fminimizer_set
gsl_multimin_fdfminimizer multimin_fminimizer_alloc
multimin_fdfminimizer_set
Other types

Any routine that allocates a gsl type can be found with Ctrl+F "alloc" and "calloc" in src/gen/gsl_export.jl and is unsafe since it returns just a pointer. These will have to be wrapped in mutable structs, given finalizers, and define cconvert, unsafe_convert pairs to the unsafe gsl type.

I will mention the following types that use callback functions in particular routines in a context where a cconvert to the callback type is safe since the function is not reused after the ccall. We shouldn't have to do anything special for these

type unsafe routines safe alternative callback type helper
gsl_cheb_series cheb_alloc GSLCheb gsl_function cheb_init
gsl_movstat_workspace movstat_alloc* gsl_movstat_function gsl_movstat_apply
gsl_monte_miser_state monte_miser_alloc gsl_monte_function monte_miser_integrate
gsl_monte_vegas_state monte_vegas_alloc gsl_monte_function monte_vegas_integrate
gsl_monte_plain_state monte_plain_alloc gsl_monte_function monte_plain_integrate

Is it correct that only the types in "Types storing callback pointers" will need to store a reference to the callback data? In particular I don't think GSLCheb needs to store a reference since gsl_cheb_series doesn't store Ptr{gsl_function}. If this is correct then I think it will become easier to auto-generate a safe interface to everything by the following logic taken from everything done so far by this pr

Procedure to create safe api
  • Start with the master branch as-is
  • Restrict existing interfaces for safety
    • Make a list of callback wrapper types from src/gen/gsl_types.jl as those having fields f::Ptr{Cvoid} or function_::Ptr{Cvoid} (could be refined, this is just based on Ctrl+F)
    • Make a list of all types from src/gen/gsl_types.jl that contain a pointer to a callback type
    • For every ccall wrapper
      • If the argument types contain a callback wrapper type and a type that contains a pointer to a callback type, add a type annotation to the callback argument to make sure it is safe
      • Otherwise add annotations to specialize on callback arguments
  • (Most likely manual) Create callback wrappers
    • For every callback function write helper functions for a wrap_callback_function(f) = ... returning (gsl_func, param_ref) that could go into a cconvert but only define the cconvert if any specialization was made for that callback function type
    • (api!) This step partly defines the api since there may be one or more callback functions and we pass some directly and others as tuples
  • Create safe api
    • Make a list of unsafe functions from src/gen/gsl_export.jl, i.e. those ending with alloc
    • Make a list of their return types based on the documentation
    • For each (function, type) pair
      • (api!) Choose a name for the new mutable struct, probably the camel-case version of the name of the allocator with the "alloc" suffix removed
      • If the return type contains a pointer to a callback wrapper type
        • Create a 3-field mutable wrapper struct with the object, parameter reference, and callback type as fields
        • For every ccall wrapper containing this type and the callback wrapper type as arguments
          • Define a method dispatching on the new struct that calls wrap_callback_function, assigns it the appropriate field and then calls the same function safely by passing the gsl_func as the callback
      • Otherwise create a 1-field mutable wrapper struct
      • Define a method of the corresponding free function for the new type that checks frees the data from the pointer by the same free function if it is not C_NULL
      • Write an inner constructor
        • calling the alloc function (maintaining the original api by passing all arguments to the allocator)
        • registering a finalizer on the new object, the corresponding free function
      • Write a cconvert, unsafe_convert pair to convert the new safe struct to the unsafe gsl type

I got carried away with writing this comment and I'll submit a pr to this pr later today adding tests for GSLCheb and GSLMinFMinimizer. Luckily the GSL documentation has many examples that can be rewritten in Julia.

@yuyichao yuyichao merged commit 921221e into master Feb 2, 2024
7 of 10 checks passed
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