search

whisperfish / rust-phonenumber

Library for parsing, formatting and validating international phone numbers.
Apache License 2.0
156 stars 55 forks source link

Fit NationalNumber in 64 bits #52

Closed rubdos closed 9 months ago

rubdos commented 1 year ago

This reduces the stack size of a PhoneNumber from 576 to 512 bits, by compressing the zeroes prefix into the value of the NationalNumber. The external API stays the same, but we win a precious 8 bytes because of alignment. If you don't have Carriers or Extensions, this is all on the stack too. Pretty cool.

We might be able to win 16 bits in Code too, but that'll need more trickery that might make it slightly less Rusty.

I got nerd sniped. Will spin up some benchmarks to make sure this is not too bad in terms of performance.

gferon commented 1 year ago

LGTM! Just curious: what brought you to this part of the code? 😄

rubdos commented 1 year ago

LGTM! Just curious: what brought you to this part of the code? smile

I was refactoring Whisperfish to use UUID and PhoneNumber instead of two strings in the database model, and then I assumed that PhoneNumber would probably be represented terribly inefficiently, compared to our previous model. I was wrong, but then spotted this. Very much nerd sniped.

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (c25b37f) 71.18% compared to head (1f46da5) 71.81%. Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #52 +/- ## ========================================== + Coverage 71.18% 71.81% +0.63% ========================================== Files 19 19 Lines 1933 1955 +22 ========================================== + Hits 1376 1404 +28 + Misses 557 551 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rubdos commented 1 year ago

Looks like it's about 10% slower on Zen 1, but there seems to be a lot of room for improving the parser performance anyway.

This is on my i5-5200U laptop:

parse/+1 520 878 2491   time:   [1.0924 µs 1.1045 µs 1.1201 µs]
                        change: [-10.809% -7.0258% -3.1920%] (p = 0.00 < 0.05)
parse/+1-520-878-2491   time:   [502.38 ns 508.15 ns 515.20 ns]
                        change: [-8.6393% -6.1105% -3.7750%] (p = 0.00 < 0.05)
rubdos commented 1 year ago

@gferon what do you think, pull this in?

rubdos commented 9 months ago

semver-checks doesn't complain, Gabriel said "LGTM", so I'm pulling it in :upside_down_face: