RFC: Use JTS PackedCoordinateSequence (Do not merge) #20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Not including GeometryCollection, because there are currently no tests for that, and it's a slippery concept in any case.
Empty points have an xyLength of 0, so this case must be checked explicitly on deserialization.
|
cc @dr-jts |
When deserializing flatgeobuff into a Java JTS geometry, we pack apart the nice `byte[]` array into a `Coordinate[]` array, which allocates many objects on the heap and scatters the memory losing data locality. JTS provides a PackedCoordinateSequence type, which stores the coordinates in a packed `double[]`. This is much more friendly to the garbage collector, and also has a smaller memory footprint (no object overhead) and is generally faster (data locality results in fewer L1/2/3 cache misses). This is a POC that uses the `PackedCoordinateSequence` instead of the default `Coordinate[]` form. It seems sad to scatter the butes after so much work for zero-copy. It'd be nice if the `byte[] -> double[]` conversion could also be zero-copy, but the only method I know to do that involves `unsafe`. NB: This does not handle Z/M coordinates at all. This is more of a conversation starter to see if this is a direction worth pursuing.
|
This definitely seems to be a good idea. |
|
Current implementation use CoordinateSequenceFilter, maybe this PR could be closed ? |
|
@nicolas-f I do not see the relation to CoordinateSequenceFilter? |
You fetch the coordinates using CoordinateSequenceFilter of Jts that iterate over coordinates instead of creating a new array. |
|
Oh sorry I was thinking it was an old code when converting from JTS. I should have looked into that when I was working on z m ordinates. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When deserializing flatgeobuff into a Java JTS geometry, we pack apart
the nice
byte[]array into aCoordinate[]array, which allocatesmany objects on the heap and scatters the memory losing data locality.
JTS provides a
PackedCoordinateSequencetype, which stores thecoordinates in a packed
double[]. This is much more friendly to thegarbage collector, and also has a smaller memory footprint (no object
overhead) and is generally faster (data locality results in fewer L1/2/3
cache misses).
This is a POC that uses the
PackedCoordinateSequenceinstead of thedefault
Coordinate[]form. It seems sad to scatter the bytes afterso much work for zero-copy. It'd be nice if the
byte[] -> double[]conversion could also be zero-copy, but the only method I know to do
that involves
unsafe.NB: This does not handle Z/M coordinates at all. This is more of a
conversation starter to see if this is a direction worth pursuing.
Do we have benchmarks that we could test the performance implications of this?