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

let ccall get the pointers using unsafe_convert #349

Merged
merged 10 commits into from
Nov 30, 2022
Merged

Conversation

visr
Copy link
Collaborator

@visr visr commented Nov 25, 2022

Adress #347, alternative approach to #348, based on discussion in #347.

Still has a few errors, like this:

Expression: AG.toWKT(AG.createlinearring([1, 2, 3], [4, 5, 6])) == AG.toWKT(AG.createlinearring([1.0, 2.0, 3.0], [4.0, 5.0, 6.0])) == "LINEARRING (1 4,2 5,3 6)"
  MethodError: no method matching ArchGDAL.IGeometry{ArchGDAL.wkbLineString}(::ArchGDAL.Geometry{ArchGDAL.wkbLineString})

  Closest candidates are:
    ArchGDAL.IGeometry{T}(::Ptr{Nothing}) where T
     @ ArchGDAL D:\visser_mn\.julia\dev\ArchGDAL\src\types.jl:251

  Stacktrace:
   [1] createlinearring(xs::Vector{Int64}, ys::Vector{Int64})
     @ ArchGDAL D:\visser_mn\.julia\dev\ArchGDAL\src\ogr\geometry.jl:1751
   [2] macro expansion
     @ D:\visser_mn\.julia\juliaup\julia-1.10.0-latest+0.x64\share\julia\stdlib\v1.10\Test\src\Test.jl:477 [inlined]
   [3] macro expansion
     @ D:\visser_mn\.julia\dev\ArchGDAL\test\test_geometry.jl:339 [inlined]

So probably we need to add some constructors to handle this case. Not sure how to best do that safely, i.e. should we clone here? If not, how do we track lifetimes?

@yeesian
Copy link
Owner

yeesian commented Nov 27, 2022

Not sure how to best do that safely, i.e. should we clone here? If not, how do we track lifetimes?

Sorry for the slow response -- I do not understand enough about Julialang's GC behavior well enough to fully follow #347. E.g. why do we need to GC.@preserve obj f(obj.ptr) where we didn't previously have to, and why does the approach of using ccall here address the issue?

I've tried reading up about it (without much success) and maybe it's better for me to ask for help/clarification here.

@visr
Copy link
Collaborator Author

visr commented Nov 27, 2022

As far as I understand, we should've used either the preserve or unsafe_convert strategy already. Now with a more aggressive GC in the new Julia version our old approach causes actual segfaults, but the unsafe situation was always there.

The problem was, when we take out the obj.ptr field, the compiler cannot see anymore that obj itself is still in use, and might call the finalizer on it. We can stop it from doing that with the preserve annotation. But we can also pass obj to ccall, and ccall is implemented in a way that does a similar annotation for us already. So that is the easier option. To be able to use that we only need to help ccall with converting obj to ptr, and pass obj instead of obj.ptr to ccall.

https://docs.julialang.org/en/v1/base/base/#Base.GC.@preserve
https://docs.julialang.org/en/v1/base/c/#Base.unsafe_convert

@yeesian
Copy link
Owner

yeesian commented Nov 28, 2022

Thank you for the explanation, really helpful! From what I understand (explained below), I agree with cloning here.

  1. I think there's something weird about the definition of linearring methods in

    @eval function createlinearring($(typedargs...))
    geom = $f1(Val{wkbLinearRing}())
    for pt in zip($(args...))
    addpoint!(geom, pt...)
    end
    return IGeometry{$T}(geom.ptr) # rewrap LinearRing as the corrent LineString/LineString25D
    end
    @eval function unsafe_createlinearring($(typedargs...))
    geom = $f1(Val{wkbLinearRing}())
    for pt in zip($(args...))
    addpoint!(geom, pt...)
    end
    return Geometry{$T}(geom.ptr) # rewrap LinearRing as the corrent LineString/LineString25D
    end

    (E.g. it's re-defining function createlinearring($(typedargs...)) for each possibility of $f1.) I have a feeling (i) $f1(Val{wkbLinearRing}()) should be unsafe_createlinestring(Val{wkbLinearRing}()), and (ii) those linearring function definitions should not be in the scope of for f in (:create, :unsafe_create).

  2. I'm okay with it being less performant for now, and find it hard to reason about how to refactor it such that their implementations are still easy to reason about, given (i) the gotchas about linearring, (ii) the use of multi-dispatch on addpoint!(...), and (iii) what's explained about memory ownership across the ccall boundary.

@visr
Copy link
Collaborator Author

visr commented Nov 28, 2022

I'm not sure exactly what to change to the createlinearring functions. If anyone has ideas, feel free to push to this branch.
Code like AG.createlinearring([1, 2, 3], [4, 5, 6]) now ultimately ends up with this error:

no method matching ArchGDAL.IGeometry{ArchGDAL.wkbLineString}(::ArchGDAL.Geometry{ArchGDAL.wkbLineString})

Is that not a kind of method that would be good to add? Reading the createlinearring functions, they are created by pushing points to an geometry and rewrapping that in a geometry with a different type parameter. I think that this is now failing because we don't directly pass pointers anymore, but a (possibly differently) typed (I)Geometry.

@rafaqz
Copy link
Collaborator

rafaqz commented Nov 28, 2022

Just to explain the special casing a little GDAL swaps the wkbLinearRing type to wkbLineString after creation, so we have to do that too with these specialised methods.

Probably adding that method is a good fix? Although I haven't totally followed these changes.

@yeesian
Copy link
Owner

yeesian commented Nov 29, 2022

If anyone has ideas, feel free to push to this branch.

I'll give it a shot and let you know when I'm done, thanks!

There isn't any logical changes here; just refactoring.
They do not follow the patterns for the other geometry types.
Namely (i) to finalize the internal geometries that were cloned and (ii) to appropriately parameterize the geometries returned.
`_infergeomtype(...)` should be operating on pointers and not geometries.
@yeesian
Copy link
Owner

yeesian commented Nov 29, 2022

If anyone has ideas, feel free to push to this branch.

I'll give it a shot and let you know when I'm done, thanks!

Back to you @visr (you can review the summary of my changes here)

@visr
Copy link
Collaborator Author

visr commented Nov 29, 2022

Great, thanks. Your changes look good to me, and fix the issue.

The remaining test failures and error are unrelated to this PR, so I'd propose to handle them in a different PR.

Specifically the error that is only on nightly "type Stateful has no field taken" is this upstream issue: JuliaIO/DiskArrays.jl#80.

The rest is related to GDAL 3.6, #345, I updated that with some more info.

@visr visr requested a review from yeesian November 29, 2022 12:32
Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work here and explanations of the remaining test failures!

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

Successfully merging this pull request may close these issues.

3 participants