[Breaking] consolidate wrapper and pointer APIs into one wrapper API

[visr]

Sorry for the massive PR. I wanted to make a PR similar to https://github.com/yeesian/ArchGDAL.jl/pull/349. By defining methods like:

Base.unsafe_convert(::Type{Ptr{Cvoid}}, x::AbstractGeometry) = x.ptr

We can let ccall know how to handle our geometry wrapper types, in a manner that is GC-safe. I haven't seen the same kind of segfault as for ArchGDAL on nightly, but nevertheless we shouldn't be vulnerable to issues like that. @jw3126 had already started addressing this by adding GC preserve macros to several places. With this approach those are not needed anymore in most places.

Right now LibGEOS.jl has geos_operations.jl with functions for wrapper types, which take out the pointer and call the counterpart function in geos_functions.jl. This essentially folds geos_operations.jl into geos_functions.jl. See for a small sample of this 53e2111113043800984ffded7ca068e89b38aa42. Rather than allowing either pointers or wrapper types, I want to shield the user from pointers as much as possible, so most of geos_functions.jl won't accept them, even if they would work. Realistically that makes this a breaking change, even though I tried to make it not to break too much code.

@jw3126 in line with #143 I tried to use operand context where possible. Some of the tests now break since they call LibGEOS.equals on two geometries with different context. I marked these with the error message "Objects have distinct GEOSContext." Perhaps at least equals works just fine even if they are from a different context?

[visr]

Thanks for your thoughts.

Can we make any definitive statements to distill/summarize this PR?

I think all 3 points are mostly valid currently, but not 100%. There are still some edge cases like clone being used on pointers in constructors, createPolygon etc. working with coordinate sequences (perhaps coordinate sequeces also need to be wrapped). These edge cases should be isolated and reduced where possible. I'm not sure if that should still be in this PR or a follow up.

Is it possible to describe what are the code changes required for users to migrate from HEAD to the version in this PR?

Yes I can work on such a list.

Might it be worthwhile to "merge" geos_functions.jl and geos_operations.jl at some point?

Yes, geos_operations.jl is now almost empty, I can put the remainder in geos_functions.jl as well. Main reason I didn't do this yet is to keep the diff somewhat readable.

[yeesian]

These edge cases should be isolated and reduced where possible. I'm not sure if that should still be in this PR or a follow up.

Thanks for the explanations! I was just wondering from a tagging of major versions perspective, I think that's fine as a follow-up. Same with the merging of geos_operations.jl and geos_functions.jl.

Yes I can work on such a list.

Thank you, I think that'll be really helpful for users looking to update! We don't have to do a CHANGELOG and having it in the top-level comment of this PR will be good enough.

[visr]

Ok, from my side this is good to go. @yeesian do you mind having another look? I added an upgrade guide to the top post, and still removed geos_operations.jl, and added deprecations for the removed prep exports.

@jw3126 regarding my comment in the top post about contexts, it seems that everything works with mixed contents. So I added b42c2be0d2fe3ba1eb5a13c796c5a8a00dbe1a92 and 27167cbb8d73e64c6e3203bbb8f18c0e4c98a645. Since mixed contents work I just always use the context of the first geometry argument. Please let me know if I'm missing something.

[jw3126]

@jw3126 regarding my comment in the top post about contexts, it seems that everything works with mixed contents. So I added b42c2be and 27167cb. Since mixed contents work I just always use the context of the first geometry argument. Please let me know if I'm missing something.

I think we had an example of an intersection that crashed with different contexts. I will try to dig it up again.

[jw3126]

I had this in mind. Maybe you modify this test to mix contexts?

[visr]

Thanks for the example. I tried, but couldn't get it to crash. For instance if I use 3 different contexts:

function f91(n)
    # adapted from https://github.com/JuliaGeo/LibGEOS.jl/issues/91#issuecomment-1267732709
    contexts = [LibGEOS.GEOSContext() for i=1:Threads.nthreads()]
    @assert Threads.nthreads() >= 3
    p = [[[-1.,-1],[+1,-1],[+1,+1],[-1,+1],[-1,-1]]]
    Threads.@threads :static for i=1:n
        ctx1 = contexts[1]
        ctx2 = contexts[2]
        ctx3 = contexts[3]
        g1 = LibGEOS.Polygon(p, ctx1)
        g2 = LibGEOS.Polygon(p, ctx2)
        for j=1:n
            @assert LibGEOS.intersects(g1, g2, ctx3)
        end
    end
    GC.gc(true)
    return nothing
end

Though thinking about it I don't see why this would be different from the single threaded case, so perhaps these tests already cover it: https://github.com/JuliaGeo/LibGEOS.jl/commit/b42c2be0d2fe3ba1eb5a13c796c5a8a00dbe1a92#diff-ea857d89ed52cb44dabd2f91748dd276a0f5e637479885bda3fcf87c6a26b227

[jw3126]

Awesome that this does not crash anymore! I agree that the single-threaded tests "obviously" cover this. OTOH multi-threading is always good for a WAT, so I think we can add this test anyway?

[visr]

Good point, I added a test in b040fd47ae4ef152b304ec1a51ab48b398fbaca5.

[jw3126]

Looking again at the test, I am confused, that this works.

    Threads.@threads :static for i=1:n
        ctx1 = contexts[1]
        ctx2 = contexts[2]
        ctx3 = contexts[3]
        g1 = LibGEOS.Polygon(p, ctx1)
        g2 = LibGEOS.Polygon(p, ctx2)
        for j=1:n
            @assert LibGEOS.intersects(g1, g2, ctx3)
        end
    end

For instance the line g1 = LibGEOS.Polygon(p,ctx1) means there could be multiple threads that try to allocate things in ctx1 at the same time.

[jw3126]

BTW a huge thanks for this PR @visr . This is an big and elegant cleanup.

[visr]

Hmm indeed after reading up a bit more in https://libgeos.org/usage/c_api/#reentrantthreadsafe-api and https://libgeos.org/development/rfcs/rfc03/ I understand why that test could be unsafe. I tried to have a look at GEOS tests if they had an example that would definitely fail, but couldn't find it. Therefore I just removed that test for now, people using threading should in general heed this advice from the docs:

Each thread that will be running GEOS operations should create its own context prior to working with the GEOS API.