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

Speed up Internet::Password generation using constants #2725

Merged
merged 1 commit into from
May 2, 2023

Conversation

MicBruz
Copy link
Contributor

@MicBruz MicBruz commented Mar 2, 2023

Summary

When reviewing #2705, I found out that there is a way to speed up password generation for Faker::Internet by more than 30% (benchmarks below). Instead of creating a new array with desired characters every time, we can use constants already defined in Faker::Base.

Also, to improve readability, I've changed [rand(special_chars.count - 1)] with a predefined sample() method and shortened if statements to base on parameters.

I changed a minimal required password length to the first password test since the default min. length is 8 (not 3)

Other Information

Benchmark I've run:

require 'benchmark'
puts Benchmark.measure { 500_000.times { Faker::Internet.password } }

Before the change I've got:
3.272090 0.010629 3.282719 ( 3.282693)

After the change I've got:
2.179874 0.005755 2.185629 ( 2.185604)

That is my first PR. I followed the guide, however, if I missed something, I am sorry!

@stefannibrasil
Copy link
Contributor

Hi @MicBruz thank you for your interest in helping Faker!

Unfortunately, this generator does not have a reliable test suite, IMO. The changes here make sense but I'm a bit hesitant to merge it as I don't trust the tests to catch any bugs as new changes are introduced.

Would you be interested in improving the tests for this generator first? I've added a comment here #2704 (comment)

Thanks!

Copy link
Contributor

@thdaraujo thdaraujo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes sense to me, there's really no need to generate new arrays for characters every time we run this method. Also, great refactoring.

Do you mind updating your fork?

password << upper_chars[rand(upper_chars.count - 1)]
if mix_case
upper_chars = self::ULetters
password << sample(upper_chars)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice refactoring!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@stefannibrasil
Copy link
Contributor

@MicBruz #2741 was merged and we now have more test coverage for edge cases, which means we can refactor with more confidence.

Are you still interested in working on this? Just need a rebase. Thanks!

character_bag += lower_chars

digits = ('0'..'9').to_a
password << digits[rand(digits.count - 1)]
password << sample(digits)
Copy link
Contributor Author

@MicBruz MicBruz Apr 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Digits appeared here so I have also adjusted that line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this a bug fix 🐞

@MicBruz
Copy link
Contributor Author

MicBruz commented Apr 29, 2023

@stefannibrasil done! Thank you for the patience and sorry for the delay :(

Copy link
Contributor

@stefannibrasil stefannibrasil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @MicBruz 🎉

@stefannibrasil stefannibrasil merged commit af05321 into faker-ruby:main May 2, 2023
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