Skip to content

remove unsafe from float.rs at very small performance cost #6

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

Closed
wants to merge 1 commit into from

Conversation

Protowalker
Copy link

OLD:

min/median/max

ns/float: 22.29/23.64/28.55
Mfloat/s: 44.86/42.31/35.03
MB/s: 781.57/737.14/610.28

NEW:
ns/float: 22.9/24.94/30.69
Mfloat/s: 43.67/40.10/32.59
MB/s: 760/87/698.71/567.77

I'm going to continue working on this to try and improve it as much as possible.

@aldanor
Copy link
Owner

aldanor commented Jan 10, 2021

Hi, thanks for attempting this :)

3% may not seem like a lot but it still is a fairly tangible difference if it's consistent (3% here, 3% there add up to quite a lot in the end). I would suggest testing it mostly on canada.txt and uniform and, as extreme case, on mesh.txt.

@shumpohl
Copy link

I totally agree that a consistent 3% advantage in such a low-level libraray justifies the usage of unsafe if unavoidable. But I think pow10_fast_path itself should be unsafe with an additional comment what the invariants are (exponent < 10) that the caller needs to uphold and a comment on unsafe block at the callside that this unsafe block is fine because you checked it beforehand.

Nice work, btw :)

@Protowalker
Copy link
Author

Can I come clean about something? I had only tested my other theory about a speed increase and not this one -- then I wrote this one, and then I made the PR, and then before I hit submit I realized I hadn't run benchmarks, so I ran some and when I realized it was slower I just changed the title from "improving performance" to "with low performance cost" 😬

I'm gonna do another implementation with the idea that I have tested but requires a larger rewrite than a couple lines, then I'll open this up again. Cheers!

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