search

georust / geographiclib-rs

A port of geographiclib in Rust.
MIT License
41 stars 9 forks source link

Update benchmarks #19

Closed michaelkirk closed 3 years ago

michaelkirk commented 3 years ago

From https://github.com/georust/geographiclib-rs/pull/17#issuecomment-763277044

I would favor also expanding the benchmarks before starting on troubleshooting the core code. This might boil down to offering the same builtin/small/full options on the rs benches that you added to the unit tests. I suggest replacing the benches' current data-copying approach with Arc+Mutex at the same time. That improved speed by about 66% in some benches I compared, though it may depend in part on the size of the test dataset.

stonylohr commented 3 years ago

@michaelkirk I'll prepare a PR for this, assuming that works for you.

michaelkirk commented 3 years ago

Please do!

On Jan 20, 2021, at 21:45, stonylohr notifications@github.com wrote:

 @michaelkirk I'll prepare a PR for this, assuming that works for you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

stonylohr commented 3 years ago

I withdraw my suggestion of using Arc and Mutex instead of cloning.

It looks like using Arc and Mutex instead of cloning inputs is a little more expensive for the builtin and short modes, and a little less expensive for the full mode. All the differences may just be noise, nothing dramatic like I saw in my earlier scenario. Thinking back on it, my inputs in the earlier scenario may have been a vector of vectors which is probably much more expensive to clone than a vector of tuples. I'll submit a PR with the remaining changes shortly.

Note that I also haven't looked into integrating it with bors yet.

It may be a couple of days until I can get back to this. You're welcome to do the bors integration yourself in the meantime, or wait til I can get back to it.

stonylohr commented 3 years ago

Actually, I used the existing CI PR, since that branch is still the only one with all the other CI-related changes. I can reformulate if you'd like it in a separate PR.

stonylohr commented 3 years ago

@michaelkirk I think we can close this. Do you agree?

michaelkirk commented 3 years ago

Agree, thanks!