new fast isValidCell #968
new fast isValidCell #968
Conversation
|
OK, current approach breaks some rules: |
|
Speedup results on my M3 mac are promising. This is for
Speedups, comparing the fastest run for each name and algo: |
|
Latest benchmarks of final algo: |
|
TODO: We test 32 bit windows in CI. Should we do the same for mac and linux? |
I don't think there is an equivalent Mac 32-bit. |
| [4] = 1, [14] = 1, [24] = 1, [38] = 1, [49] = 1, [58] = 1, | ||
| [63] = 1, [72] = 1, [83] = 1, [97] = 1, [107] = 1, [117] = 1}; | ||
|
|
||
| if (isBaseCellPentagonArr[base_cell]) { |
There was a problem hiding this comment.
Is there a reason not to use (perhaps an inlined version of) _isBaseCellPentagon?
There was a problem hiding this comment.
Good point. I'll run some benchmarks and see if there's any difference.
There was a problem hiding this comment.
Definitely slower with the naive use of _isBaseCellPentagon. Test setup in the latest commit:
I'll figure out the right extern and inline incantation and try that next.
Another alternative might be to just expose the BaseCellData directly.
More generally for the H3 lib as a whole, I'm not sure if there's a benefit to using a struct of arrays vs an array of structs.
There was a problem hiding this comment.
I think doing this will be a little involved, so I'm proposing doing it later :)
Think that's a blocker for this PR, or can I create a separate task/PR to add that to CI? @isaacbrodsky |
I don't think it's a blocker, but we could certainly merge it in first. My sense is Linux x86/i*86 is much less common today and probably not an architecture we need to be too concerned with testing. I'll open a PR to include Linux arm64. I don't think Github provides Linux arm32 runners, at least as standard: https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories edit: #974 |
|
Issues trying to inline: |
|
I'm not super familiar with inline functions in C, but going by this StackOverflow answer you need |
Nice. That works for me locally, but with some errors. Let's see now it does in CI. |
f331d1a to
52c7146
Compare
|
Note the follow-up: #984 |
Taking over from #496.
New
isValidCellshould be faster in all cases. Relies on bit manipulation instead of loops.In the last correctness check, we use compiler intrinsics, so we'll want to verify that that approach works on all platforms. We can always fall back to a loop, and that would still be faster (see the benchmarks plot below). However, we need to ensure that we have the right test to evaluate which approaches are available on each platform. I'm not currently sure how to do that in an exhaustive way.