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

Fit NationalNumber in 64 bits #52

Merged
merged 3 commits into from
Dec 14, 2023
Merged

Fit NationalNumber in 64 bits #52

merged 3 commits into from
Dec 14, 2023

Conversation

rubdos
Copy link
Member

@rubdos rubdos commented Apr 11, 2023

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.

@rubdos rubdos changed the title Fit NationalNumber in 64 bits Draft/RFC: Fit NationalNumber in 64 bits Apr 11, 2023
@rubdos rubdos mentioned this pull request Apr 18, 2023
@gferon
Copy link
Contributor

gferon commented Apr 18, 2023

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

@rubdos
Copy link
Member Author

rubdos commented Apr 18, 2023

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
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rubdos rubdos marked this pull request as ready for review June 20, 2023 09:48
@rubdos rubdos force-pushed the compact-national branch 2 times, most recently from df8eb32 to 7d03bc9 Compare June 20, 2023 10:16
@rubdos
Copy link
Member Author

rubdos commented Jun 20, 2023

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 rubdos requested a review from gferon June 20, 2023 12:23
@rubdos
Copy link
Member Author

rubdos commented Aug 22, 2023

@gferon what do you think, pull this in?

@rubdos rubdos changed the title Draft/RFC: Fit NationalNumber in 64 bits Fit NationalNumber in 64 bits Sep 5, 2023
@rubdos
Copy link
Member Author

rubdos commented Dec 14, 2023

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

@rubdos rubdos merged commit 928bb3f into main Dec 14, 2023
8 checks passed
@rubdos rubdos deleted the compact-national branch December 14, 2023 14:17
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