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

Reduce overhead. #338

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Reduce overhead. #338

merged 3 commits into from
Nov 20, 2024

Conversation

FreddieWitherden
Copy link
Contributor

This PR aims to reduce API overhead. For small in-memory trees the benefits can be substantial (30% or more). General ideas:

  • Function calls with ctypes are expensive (1µs per call). So, we add caching to the tree type and index dimension properties. This greatly cuts down on the number of times we need to call into the library.
  • The bulk loading routine calls deinterleave which is slow and does a lot of ctypes casting. This replaces it with slice assignment which is more direct and faster (for my test case tree construction goes from ~10s to ~7s).
  • We also adopt slice assignment for get_coordinate_pointers. There should now be virtually no overhead from not interleaving. Moreover, point arguments should be a bit faster as we avoid some work by returning the same pointer twice.

All test pass on my end, but some more cross-checking would be useful from people who have more experience with the library. Overhead is still greater than I would like but a good chunk of this is baked in due to the C API of libspatialindex (namely how indices are returned which should really be caller allocated rather than callee allocated).

@hobu
Copy link
Member

hobu commented Nov 18, 2024

baked in due to the C API of libspatialindex

I would be happy to merge PRs to libspatialindex that added (not changed) methods to flip the allocation behavior.

@FreddieWitherden
Copy link
Contributor Author

baked in due to the C API of libspatialindex

I would be happy to merge PRs to libspatialindex that added (not changed) methods to flip the allocation behavior.

Okay, I will work something up. The very rough idea: have the caller pass in a pre-allocated array along with its size. Then the routine will replace the size with the number of entries it wanted to copy in. If the size of the array we passed in is greater than or equal to this then we're good, otherwise we reallocate our internal buffer and call the function again. The array itself can likely come from the array module and can be kept around as a member variable.

This will save a function call and some allocations. I am unsure if the object APIs should be handled this way too as the overhead there is just quite a bit larger anyway.

@FreddieWitherden
Copy link
Contributor Author

It will take a while for me to get the new API in place but if there are no objections I think this can be merged? It provides a nice performance boost as-is without requiring a new version of libspatialindex.

@hobu
Copy link
Member

hobu commented Nov 20, 2024

I think this can be merged

Need to clean up the missing type annotation stuff. https://results.pre-commit.ci/run/github/1919660/1731903099.leoDa_rGT-GZIOWd00LSpQ

@FreddieWitherden
Copy link
Contributor Author

I think this can be merged

Need to clean up the missing type annotation stuff. https://results.pre-commit.ci/run/github/1919660/1731903099.leoDa_rGT-GZIOWd00LSpQ

Should now be resolved.

@hobu hobu merged commit 1b6f141 into Toblerity:master Nov 20, 2024
20 checks passed
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.

2 participants