Skip to content

Conversation

@dbaston
Copy link
Member

@dbaston dbaston commented Dec 10, 2022

This PR modifies HotPixelIndex to back it with an STRtree instead of a KdTree. It provides about a 15% performance gain in a buffer benchmark that heavily uses the HotPixelIndex. I don't know if that's worth the change. Peak memory usage increases by about 10%. There may be room for more tuning.

gps_tracks.txt

bin/geosop -t -q -a ~/data/gps_tracks.txt buffer 20

(test case is from r-spatial/sf#2039)

This provides about a 15% performance gain in a buffer benchmark that heavily uses the HotPixelIndex.
dbaston added a commit to dbaston/libgeos that referenced this pull request Dec 13, 2022
@pramsey
Copy link
Member

pramsey commented Feb 3, 2023

Is there any implementation improvement left to wring out of kd-tree? The "conventional wisdom" on kd-tree is that for points it should be superior to r-tree in principle. If it's not in practice that would be an important learning.

@dbaston
Copy link
Member Author

dbaston commented Feb 4, 2023

I poked at this a bit, writing a KdTree implementation that uses bulk loading so it can be fully balanced, with nodes in a contiguous block of memory. That gets us most of the performance gain of switching to STRtree but is still a bit slower. And then you could probably make STRtree faster, either using float envelopes (#757) or a SIMD implementation (dbaston@7af0847).

@pramsey
Copy link
Member

pramsey commented May 4, 2023

I think you proved the point: it's faster, it's not worse. Why not merge?

@dbaston
Copy link
Member Author

dbaston commented May 4, 2023

Why not merge?

Merge conflict, modest benefits, general conservatism about differences from JTS

@dbaston dbaston added this to the 3.13.0 milestone Jun 7, 2023
@robe2 robe2 modified the milestones: 3.13.0, 3.14.0 Aug 21, 2024
@dbaston dbaston modified the milestones: 3.14.0, 3.15.0 Jul 11, 2025
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