-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Benchmark revamp + run benchmark as part of CI #3176
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
Conversation
8a7e4f2 to
a937aac
Compare
0d24c5e to
ca35c75
Compare
Inspired by ruby/json#606
Having these in a folder helps because we can document experiment results in it as well. And we can edit the require script to raise an error if it takes longer than a threshold to load faker. Co-Authored-By: Thiago Araujo <thd.araujo@gmail.com>
5fee76f to
a109a55
Compare
| branches: [ main ] | ||
| pull_request: | ||
| branches: [ main ] | ||
|
|
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
| permissions: | |
| contents: read | |
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.
Fair enough. I also added an extra permission for pull requests, and for the tests workflow as well in 07769e9
| x.report("Number of generators: #{all_generators.count}") do | ||
| all_generators.each { |generator| eval(generator) } | ||
| end |
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.
we should build the list of generators outside so that we're only benchmarking generator execution
| x.report("Number of generators: #{all_generators.count}") do | |
| all_generators.each { |generator| eval(generator) } | |
| end | |
| generators = all_generators | |
| x.report("Number of generators: #{all_generators.count}") do | |
| generators.each { |klass, generator| klass.send(generator) } | |
| end |
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.
Good catch, thanks! Changed in 8714ae3
benchmark/load_yml_vs_json.rb
Outdated
| x.report('JSON') { JSON.load_file("#{File.dirname(__FILE__)}/../test/fixtures/locales/es-MX.json") } | ||
|
|
||
| x.compare!(order: :baseline) | ||
| end |
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.
Do we still want to keep this? Benchmarking json vs yaml load times is not really relevant to Faker, this was just an experiment.
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.
Good point. I was only copying things over from the benchmark rake task. Removed it in 7745001
.rubocop.yml
Outdated
| Description: The use of eval represents a serious security risk. | ||
| Exclude: | ||
| - 'lib/faker/default/json.rb' | ||
| - 'benchmark/generators.rb' |
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 we don't need eval on generators, see previous comment
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.
Done in 8714ae3
thdaraujo
left a comment
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.
Nice work on this! do you mind adding to the description the machine you're using so we can compare apples to apples?
e.g. Apple M1 16GB memory on MacOS X.Y.Z
… execution Looping over the constants instead of using `eval` is a more secure approach. Plus, build the list of generators outside so that we're only benchmarking generator execution. Co-Authored-By: Thiago Araujo <thd.araujo@gmail.com>
Yes! Added to the PR description. |
thdaraujo
left a comment
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.
very nice, thank you!
Motivation / Background
We want to run some experiments to improve the library's performance needs and before making any changes, we need to have baseline stats to guarantee new code does not degrade performance. Plus, we want to have benchmark scripts to eventually be part of our CI.
To keep everything in a single place, I moved the previous benchmark tasks to a folder. The goal is to use the folder to document experiment results.
Closes #3159, #3160
Inspired by ruby/json#606.
Results from running the scripts (January 12th, 2026). Machine specs:
Apple M1 Pro 16GB memory on MacOS Sequoia 15.7.3.Require
Load locales - YML vs JSON
Generators