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

Allocations in convert(Ptr{T}, Handle{T}) #435

Closed
staticfloat opened this issue Nov 18, 2023 · 3 comments
Closed

Allocations in convert(Ptr{T}, Handle{T}) #435

staticfloat opened this issue Nov 18, 2023 · 3 comments

Comments

@staticfloat
Copy link
Contributor

We appear to be getting unnecessary allocations due to the implementation of Handle{T}. In particular:

julia> @code_warntype convert(Ptr{Sundials.IDAMem}, integ.mem)
MethodInstance for convert(::Type{Ptr{Sundials.IDAMem}}, ::Sundials.Handle{Sundials.IDAMem})
  from convert(::Type{Ptr{T}}, h::Sundials.Handle{T}) where T @ Sundials ~/.julia/dev/Sundials/src/handle.jl:86
Static Parameters
  T = Sundials.IDAMem
Arguments
  #self#::Core.Const(convert)
  _::Core.Const(Ptr{Sundials.IDAMem})
  h::Sundials.Handle{Sundials.IDAMem}
Body::Any
1 ─ %1 = Base.getproperty(h, :ptr_ref)::Ref{Ptr{Sundials.IDAMem}}
│   %2 = Base.getindex(%1)::Any
└──      return %2

The ::Any from h.ptr_ref[] seems to force the allocation of a Box, and since this happens every time we ask the integrator for a state vector, it can add up to many thousands of allocations per solve, and if you're doing a large parameter sweep of simple systems, you can quickly spend more time GC'ing than solving.

@staticfloat staticfloat changed the title Allocations due to Handle{T} Allocations in convert(Ptr{T}, Handle{T}) Nov 18, 2023
@staticfloat
Copy link
Contributor Author

@vtjnash Is this a fundamental limitation of Ref? Minimizing the example:

struct Handle{T}
    ptr_ref::Ref{Ptr{T}}
end
Base.unsafe_convert(::Type{Ptr{T}}, h::Handle{T}) where {T} = h.ptr_ref[]

h = Handle{Int}(Ref(Ptr{Int}(0)))
@code_warntype Base.unsafe_convert(Ptr{Int}, h)

Shows an ::Any where I would expect a Ptr{Int}:

MethodInstance for Base.unsafe_convert(::Type{Ptr{Int64}}, ::Handle{Int64})
  from unsafe_convert(::Type{Ptr{T}}, h::Handle{T}) where T @ Main REPL[26]:4
Static Parameters
  T = Int64
Arguments
  #self#::Core.Const(Base.unsafe_convert)
  _::Core.Const(Ptr{Int64})
  h::Handle{Int64}
Body::Any
1 ─ %1 = Base.getproperty(h, :ptr_ref)::Ref{Ptr{Int64}}
│   %2 = Base.getindex(%1)::Any
└──      return %2

@vtjnash
Copy link

vtjnash commented Nov 18, 2023

It is an abstract type. A mutable struct usually makes more sense, since it performs better (if you don't need to pass it to C) and allocating a new Ref makes more sense if you do need to pass to C (since it can be stack allocated)

@staticfloat
Copy link
Contributor Author

So if I'm understanding you correctly, you're suggesting Handle above instead be implemented as:

mutable struct Handle{T}
    ptr::Ptr{T}
end
Base.unsafe_convert(::Type{Ptr{T}}, h::Handle{T}) where {T} = h.ptr

h = Handle{Int}(Ptr{Int}(0))

sjdaines added a commit to sjdaines/Sundials.jl that referenced this issue Jan 27, 2024
Addresses SciML#435 and
also fixes a potential problem with memory safety:

1) Allocations in convert(Ptr{T}, Handle{T})
   Fix is as suggested in SciML#435
   (Handle is now a mutable struct with a ptr field)

2) Memory safety robustness fix
   Remove convert and use paired Base.cconvert / Base.unsafe_convert to get the ptr field from
   a Handle object h, so that the h is preserved from GC across the ccall
   This fix is analogous to that for the NVector wrapper in PR SciML#380
   (NB: there is no actual problem at least when Sundials is used with the SciML interface,
   as the Handle{CVODEMem} and similar objects are held by a persistent solver data structure, but this
   change should reduce the risk of something going wrong in the future, or for eg test harnesses that don't use
   the SciML interface)
ChrisRackauckas pushed a commit that referenced this issue Jan 27, 2024
Addresses #435 and
also fixes a potential problem with memory safety:

1) Allocations in convert(Ptr{T}, Handle{T})
   Fix is as suggested in #435
   (Handle is now a mutable struct with a ptr field)

2) Memory safety robustness fix
   Remove convert and use paired Base.cconvert / Base.unsafe_convert to get the ptr field from
   a Handle object h, so that the h is preserved from GC across the ccall
   This fix is analogous to that for the NVector wrapper in PR #380
   (NB: there is no actual problem at least when Sundials is used with the SciML interface,
   as the Handle{CVODEMem} and similar objects are held by a persistent solver data structure, but this
   change should reduce the risk of something going wrong in the future, or for eg test harnesses that don't use
   the SciML interface)
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

No branches or pull requests

3 participants