aboutsummaryrefslogtreecommitdiff
path: root/julia/CrystFEL
diff options
context:
space:
mode:
authorThomas White <taw@physics.org>2024-01-03 16:46:10 +0100
committerThomas White <taw@physics.org>2024-02-06 16:59:35 +0100
commit1f2185b46932d2a3c107a3f823b90a46a84ce202 (patch)
treef63c9386893208a0be2f1480f1f3ff632e67d67e /julia/CrystFEL
parent8b67ec27c563d35fa8c5604b3475ff7b017abcb2 (diff)
Julia: fix Image/PeakList memory semantics
The previous commit 3bb46f49 was a mistake. We can't just free the old peak list, because there might still be a PeakList object around that refers to it. This new version keeps a record of the PeakList object itself, setting in_image=false if a new list is added, so that it can be freed by the GC. I made an attempt at making this thread-safe, but I think it's impossible to get it completely correct. One should simply avoid setting any Image fields in the same structure from different threads.
Diffstat (limited to 'julia/CrystFEL')
-rw-r--r--julia/CrystFEL/src/image.jl44
-rw-r--r--julia/CrystFEL/src/peaklist.jl7
2 files changed, 28 insertions, 23 deletions
diff --git a/julia/CrystFEL/src/image.jl b/julia/CrystFEL/src/image.jl
index de46af4b..24c1c923 100644
--- a/julia/CrystFEL/src/image.jl
+++ b/julia/CrystFEL/src/image.jl
@@ -41,6 +41,7 @@ end
mutable struct Image
internalptr::Ptr{InternalImage}
+ peaklist::Union{Nothing,PeakList}
end
@@ -65,19 +66,12 @@ end
function Base.getproperty(image::Image, name::Symbol)
if name === :internalptr
getfield(image, :internalptr)
+ elseif name === :peaklist
+ getfield(image, :peaklist)
else
idata = unsafe_load(image.internalptr)
- if name === :peaklist
- let pl = getproperty(idata, :peaklist)
- if pl == C_NULL
- throw(ErrorException("Image doesn't have a peak list"))
- else
- PeakList(pl, true)
- end
- end
-
- elseif name === :crystals
+ if name === :crystals
return makecrystallist(getproperty(idata, :crystals),
getproperty(idata, :n_crystals))
@@ -98,20 +92,26 @@ function assert_type(val, type)
end
-function set_peaklist(image, val)
- assert_type(val, PeakList)
- if val.in_image
+function set_peaklist(image, new_pl)
+
+ assert_type(new_pl, PeakList)
+
+ if new_pl.in_image
throw(ArgumentError("PeakList is already in an image. "*
"Add a copy (use `deepcopy`) instead."))
end
- val.in_image = true
- val = val.internalptr
- idata = unsafe_load(image.internalptr)
- old = swapproperty!(idata, :peaklist, val)
- unsafe_store!(image.internalptr, idata)
+ new_pl.in_image = true
+ old_pl = swapfield!(image, :peaklist, new_pl)
+ if old_pl !== nothing
+ old_pl.in_image = false
+ # The old PeakList will now get GCed
+ end
- if old != C_NULL
- @ccall libcrystfel.image_feature_list_free(old::Ptr{InternalPeakList})::Cvoid
+ idata = unsafe_load(image.internalptr)
+ old_i = swapfield!(idata, :peaklist, new_pl.internalptr)
+ if old_pl === nothing || old_i == old_pl.internalptr
+ unsafe_store!(image.internalptr, idata)
+ # else someone else already set a new peaklist
end
end
@@ -180,7 +180,7 @@ function Image(dtempl::DataTemplate)
throw(ArgumentError("Failed to create image"))
end
- image = Image(out)
+ image = Image(out, nothing)
finalizer(image) do x
ccall((:image_free, libcrystfel), Cvoid, (Ptr{InternalImage},), x.internalptr)
@@ -212,7 +212,7 @@ function Image(dtempl::DataTemplate,
throw(ArgumentError("Failed to load image"))
end
- image = Image(out)
+ image = Image(out, nothing)
finalizer(image) do x
ccall((:image_free, libcrystfel), Cvoid, (Ptr{InternalImage},), x.internalptr)
diff --git a/julia/CrystFEL/src/peaklist.jl b/julia/CrystFEL/src/peaklist.jl
index 1735de5e..a676a7bf 100644
--- a/julia/CrystFEL/src/peaklist.jl
+++ b/julia/CrystFEL/src/peaklist.jl
@@ -25,7 +25,12 @@ function PeakList()
if out == C_NULL
throw(ArgumentError("Failed to create peak list"))
end
- PeakList(out, false)
+ finalizer(PeakList(out, false)) do pl
+ if !pl.in_image
+ @ccall libcrystfel.image_feature_list_free(pl.internalptr::Ptr{InternalPeakList})::Cvoid
+ # else it belongs to the image structure
+ end
+ end
end