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.

Sign up for GitHub

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

Jump to bottom

Swap to returning Tuples instead of LibGEOS.Point for anything GeoInterface related #190

Open
rafaqz opened this issue Dec 28, 2023 · 5 comments · May be fixed by #193
Open

Swap to returning Tuples instead of LibGEOS.Point for anything GeoInterface related #190

rafaqz opened this issue Dec 28, 2023 · 5 comments · May be fixed by #193

Comments

@rafaqz
Copy link
Member

rafaqz commented Dec 28, 2023 •
edited
Loading

This is likely related to the memory leak too. We are creating LibGEOS Point objects for every single point we read in a geometry, but they're incredibly expensive and allocate objects that need to be finalized:

LibGEOS.jl/src/geos_types.jl

Lines 29 to 50 in 7ad3b3c

mutable struct Point <: AbstractGeometry
ptr::GEOSGeom
context::GEOSContext
# create a point from a pointer - only makese sense if it is a pointer to a point, otherwise error
function Point(obj::Union{Point,GEOSGeom}, context::GEOSContext = get_global_context())
id = LibGEOS.geomTypeId(obj, context)
point = if id == GEOS_POINT
point = new(cloneGeom(obj, context), context)
else
open_issue_if_conversion_makes_sense(Point, id)
end
finalizer(destroyGeom, point)
point
end
# create a point from a vector of floats
Point(coords::Vector{Float64}, context::GEOSContext = get_global_context()) =
Point(createPoint(coords, context), context)
Point(x::Real, y::Real, context::GEOSContext = get_global_context()) =
Point(createPoint(x, y, context), context)
Point(x::Real, y::Real, z::Real, context::GEOSContext = get_global_context()) =
Point(createPoint(x, y, z, context), context)
end

Which leads to insane performance like this:
JuliaGeo/GeometryOps.jl#32

74 allocations to get the area of an 11 point geometry.

We also call getPoint :

LibGEOS.jl/src/geos_functions.jl

Lines 1516 to 1546 in 7ad3b3c

# Call only on LINESTRING, and must be freed by caller (Returns NULL on exception)
function getPoint(
obj::Union{LineString,LinearRing},
n::Integer,
context::GEOSContext = get_context(obj),
)
if !(0 < n <= numPoints(obj, context))
error(
"LibGEOS: n=$n is out of bounds for LineString with numPoints=$(numPoints(obj, context))",
)
end
result = GEOSGeomGetPointN_r(context, obj, n - 1)
if result == C_NULL
error("LibGEOS: Error in GEOSGeomGetPointN")
end
Point(result, context)
end
# Call only on LINESTRING, and must be freed by caller (Returns NULL on exception)
function startPoint(
obj::Union{LineString,LinearRing},
context::GEOSContext = get_context(obj),
)
result = GEOSGeomGetStartPoint_r(context, obj)
if result == C_NULL
error("LibGEOS: Error in GEOSGeomGetStartPoint")
end
Point(result, context)
end
# Call only on LINESTRING, and must be freed by caller (Returns NULL on exception)

We should replace all of this with just getting two floating point values in the simplest way possible, and getting them all at once for whole LinearRing/LineString in GI.getpoint(linestring)
@evetion
Copy link
Member

evetion commented Jan 1, 2024

After #191 it's at least type stable, but indeed, much copying is done.
Screenshot 2024-01-01 at 14 52 21

@rafaqz
Copy link
Member Author

rafaqz commented Jan 1, 2024 •
edited
Loading

I think it can be 50x faster, for linestrings anyway

@evetion
Copy link
Member

evetion commented Jan 1, 2024

Sure, but not sure whether this is the right place to do so. Libgeos should be consistent. What could work is making a single efficient copy to a GI Polygon/Ring for Ops to work on?

@rafaqz
Copy link
Member Author

rafaqz commented Jan 1, 2024 •
edited
Loading

We already did this for ArchGDAL for the same reason.

We only need to be consistent with GeoInterface in GeoInterface methods.

The idea is to do what your saying, but how could you do that in GeometryOps without a LibGeos dep?

The only way I can see to do it is here in GI.getgeom on the iterator over LineString

@rafaqz rafaqz linked a pull request Jan 1, 2024 that will close this issue
Open
@evetion
Copy link
Member

evetion commented Jan 2, 2024

I like to keep packages internally consistent as well. I missed the ArchGDAL change, let me check that.

Maybe this isn't needed with all the other activities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

return tuple points from GeoInterface methods JuliaGeo/LibGEOS.jl
2 participants
@rafaqz @evetion