-
Notifications
You must be signed in to change notification settings - Fork 10
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
Optimize is_card_number to use array #678
Conversation
c9453f0
to
60e0f3b
Compare
BenchmarksComparisonBenchmark execution time: 2024-10-22 06:28:04 Comparing candidate commit 1955f2e in PR branch Found 23 performance improvements and 0 performance regressions! Performance is the same for 28 metrics, 2 unstable metrics. scenario:benching deserializing traces from msgpack to their internal representation
scenario:credit_card/is_card_number/ 3782-8224-6310-005
scenario:credit_card/is_card_number/ 378282246310005
scenario:credit_card/is_card_number/378282246310005
scenario:credit_card/is_card_number/37828224631000521389798
scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/ 3782-8224-6310-005
scenario:credit_card/is_card_number_no_luhn/ 378282246310005
scenario:credit_card/is_card_number_no_luhn/378282246310005
scenario:credit_card/is_card_number_no_luhn/37828224631000521389798
scenario:credit_card/is_card_number_no_luhn/x371413321323331
scenario:redis/obfuscate_redis_string
scenario:tags/replace_trace_tags
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
BaselineOmitted due to size. |
60e0f3b
to
45b1728
Compare
45b1728
to
850a88a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #678 +/- ##
==========================================
+ Coverage 71.75% 71.80% +0.05%
==========================================
Files 271 271
Lines 40992 40982 -10
==========================================
+ Hits 29414 29429 +15
+ Misses 11578 11553 -25
|
850a88a
to
c6a567f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct to me.
@@ -11,19 +11,29 @@ pub fn is_card_number<T: AsRef<str>>(s: T, validate_luhn: bool) -> bool { | |||
return false; | |||
} | |||
|
|||
let mut num_s = Vec::with_capacity(s.len()); | |||
let mut num_s = [0; 17]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a debug assert that s.len() <= num_s.len()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, s.len()
can be longer than 17
but we stop after 17
digits in the loop below.
c6a567f
to
1955f2e
Compare
What does this PR do?
Use a fixed size array (we know that credit card numbers are at most 16 digits long) instead of a dynamic
Vec
.Motivation
While looking at benchmark fluctuations, I saw that the code seemed to do unnecessary operations. Why not optimize it?