Closed rafaqz closed 10 months ago
Yeah, this is something I encountered as well and partially fixed for points, just like you in the other PR.
There are double inits, as getX already is a X. Most constructors do clone if given the chance.
Clonegeom is only needed on init with another existing geometry.
However, we must be careful. Your exteriorring instance must be cloned per https://libgeos.org/doxygen/geos__c_8h.html#afc5d54a2905996833747c2d566da2af3.
I will make a PR
I wondered... instead of cloning could we skip the finaliser on polygon rings?
The important thing is not to delete them?
(I guess you could get a ring then delete its parent polygon and trigger a segfault)
Are you proposing we leak memory? ;) Everything we create in GEOS needs to be destroyed again.
We arent creating anything with polygon rings, and theyre attached to the polygons finaliser. Right? Why else should we never delete them?
Ah, I misunderstood. You meant like a view or similar? The problem is that Julia doesn't know the ring is linked to the Polygon, so it could garbage collect the Polygon, invalidating the ring.
We could make structs like prepared geometry, linking back to the Polygon object?
Anyway, not sure how much performance you gain by it.
I think its more than half the cost of loading a polygon. Probably we would need an GI.unsafe_getgeom
method where users promise the objects dont escape and we GC.preserve the polygon around a closure.
One day when everything else is done 😅
Let's not burden GI with unsafe, but let's see what I can make with a GC.preserve, never used it extensively.
Well, with the following code (including a third argument to the constructor to run a finalizer or not), the tests run, but I can trigger segfaults. Any ideas?
GC.@preserve obj begin
return LinearRing(result, context, false)
end
But this segfaults:
julia> import LibGEOS as LG
julia> ring = LG.exteriorRing(pLG)^C
julia> pLG = LG.Polygon([
[
[0.0, 5.0], [2.0, 2.0], [5.0, 2.0], [2.0, -2.0], [5.0, -5.0],
[0.0, -2.0], [-5.0, -5.0], [-2.0, -2.0], [-5.0, 2.0], [-2.0, 2.0],
[0.0, 5.0],
],
])
POLYGON ((0 5, 2 2, 5 2, 2 -2, 5 -5, 0 -2, -5 -5, -2 -2, -5 2, -2 2, 0 5))
julia> ring = LG.exteriorRing(pLG)
LINEARRING (0 5, 2 2, 5 2, 2 -2, 5 -5, 0 -2, -5 -5, -2 -2, -5 2, -2 2, 0 5)
julia> pLG = nothing
julia> GC.gc()
julia> ring
# Segfaults
Thats why we need a method with a promise the contents dont escape the closure! I dont think it can work otherwise
Or something like this, with AbstractLinearRing replacing LinearRing in most signatures.
abstract type AbstractLinearRing <: AbstractGeometry end
struct LinearRingView <: AbstractLinearRing
ptr::GEOSGeom
context::GEOSContext
parent::AbstractGeometry
function LinearRingView(
obj::GEOSGeom,
parent::AbstractGeometry,
context::GEOSContext = get_global_context(),
)
new(obj, context, parent)
end
end
mutable struct LinearRing <: AbstractLinearRing
Wont it still segfault if you delete the polygon?
Nope, because the polygon is linked in the parent field.
We often seem to do this multiple times. Clearly we can remove some of these double clones. But do we need to clone at all? I cant see why...
This one seems like an obvious unnecessary copy:
https://github.com/JuliaGeo/LibGEOS.jl/blob/feb0055bebe72fed41ac67363c91a9bc5350e90b/src/geos_functions.jl#L1416
Because then we copy again here:
https://github.com/JuliaGeo/LibGEOS.jl/blob/feb0055bebe72fed41ac67363c91a9bc5350e90b/src/geos_types.jl#L179
Removing these copies makes everything much faster. This is our GeometryOps.jl
area
method` (with a few branches checked out) on LG polygon. We don't need LibGEOS, its just for testing. But its a good example:https://github.com/asinghvi17/GeometryOps.jl/issues/32
With
cloneGeom
Removing the first
cloneGeom
:And the first and second
cloneGeom
And finally if I remove the one one the line above when a
LineString
rewraps anotherLineString
:We shouldn't change this one, but it shouldn't be happening in
getgeom
anyway.