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

Improve WKB performance. #39

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve WKB performance. #39

wants to merge 1 commit into from

Conversation

evetion
Copy link
Owner

@evetion evetion commented Oct 6, 2024

Fixes #32. Points are now Tuples, so this will be a breaking release (will release a minor release first for the other PRs).

NTuple is type unstable if ncoord is not known, so let's make it Val. Shortcutting to GI.ncoord(T, geom) also helps a lot of unnecessary geomtrait calls that are already known. Writing out geomtrait also helped.

@rafaqz
Copy link
Collaborator

rafaqz commented Oct 6, 2024

At some point we will need wkb geometry types that store ncoord/hasm/hasz in the type for wkb to be fast (we could just put them in GI wrappers )

Using Val at the getcoord level isn't so much of a performance gain as we are still lifting runtime values to types in inner loops. Ultimately you want type stability at the level of a whole polygon, or better for a vector of polygons.

Also in terms of GeometryOps or Rasters performance the key is defining GI.x and GI.y manually, that will skip around much of the problem.

@evetion
Copy link
Owner Author

evetion commented Oct 6, 2024

At some point we will need wkb geometry types that store ncoord/hasm/hasz in the type for wkb to be fast (we could just put them in GI wrappers )

Using Val at the getcoord level isn't so much of a performance gain as we are still lifting runtime values to types in inner loops. Ultimately you want type stability at the level of a whole polygon, or better for a vector of polygons.

Also in terms of GeometryOps or Rasters performance the key is defining GI.x and GI.y manually, that will skip around much of the problem.

WKB is a storage type, not a good in memory type, that's why I never made it a struct on its own, just a good enough parser/serializer. Feel free to wrap them, or even convert them to somewhere else.

Agreed that a single, topmost ncoord call should be ideal, but that requires rewriting this stuff without GeoInterface. At the moment the Val removed the type instability of NTuple (also in the return type) and stuff has become much more performant.

I will add manual xyzm methods here.

@@ -219,7 +224,7 @@ function GI.getgeom(
geom::WKBtype,
i::Integer,
)
ncoord = GI.ncoord(geom)
ncoord = GI.ncoord(T, geom)
GI.getgeom(T, geom, i, ncoord)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be an internal method rather than adding non-standard methods to GI.getgeom ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah, not part of this PR, but I should fix that yes. It seems I already thought of your suggestion to pass ncoord form the toplevel in a hacky way.

@rafaqz
Copy link
Collaborator

rafaqz commented Oct 6, 2024

Adding specialised getgeom(trait, geom) methods to return an iterator could also help performance

@rafaqz
Copy link
Collaborator

rafaqz commented Oct 6, 2024

WKB is a storage type, not a good in memory type

Mostly I'm just thinking about minimising read time

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.

Efficient coordinate collection from LineStrings using reinterpret
2 participants