-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add benchamark #77
Add benchamark #77
Conversation
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.
@PikachuEXE - Thanks for the contribution! Adding some benchmarks to this project is a great idea. Do you know of any good tools that would allow us to generate good benchmark reports both with and without the goldiloader gem loaded since the ActiveRecord monkey patching could affect performance even with explicit eager loads?
benchmark/main.rb
Outdated
Post.create!(author: user_2, blog: blog_2) | ||
end | ||
|
||
Benchmark.bm(20) do |x| |
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.
Should we use benchmark-ips to add benchmark warm up, generate results with better statistical significance and provide better reporting of the difference?
benchmark/main.rb
Outdated
Benchmark.bm(20) do |x| | ||
# Use AR's eager loading | ||
x.report("AR eager loading: ") do | ||
::Blog.all.includes(posts: :author).map do |blog| |
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.
Should this be an each
rather than a map
? It doesn't look like the code does anything with the result.
benchmark/main.rb
Outdated
# Use AR's eager loading | ||
x.report("AR eager loading: ") do | ||
::Blog.all.includes(posts: :author).map do |blog| | ||
blog.posts.to_a.each do |post| |
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.
I'm not sure it matters but this benchmark will generate less garbage if we drop the to_a
and call blog.posts.each
.
fbca601
to
0420db3
Compare
Benchmark updated
|
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.
Thanks for the updates @PikachuEXE. The benchmark results look a lot more useful using benchmark-ips. The Travis builds are failing due to some Rubocop violations: https://travis-ci.org/salsify/goldiloader/jobs/463169992. A few small comments and then we can get this merged.
@@ -0,0 +1,120 @@ | |||
# frozen_string_literal: true | |||
|
|||
ActiveRecord::Schema.define(version: 0) do |
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.
It looks like this was copied from spec/db/schema.rb
. If we think benchmarks and correctness tests will use the same schemas can we share that file across both types of tests. Alternatively we think the schemas for benchmarks and correctness tests will diverge, can we slim down benchmark/db/schema.rb
to just the schema elements needed by the current benchmarks.
benchmark/main.rb
Outdated
|
||
# Use goldiloader | ||
x.report("AR with goldiloader: ") do | ||
::Blog.all.map do |blog| |
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.
This map
should be an each
to be consistent with the "AR eager loading" test.
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.
These benchmarks are already proving useful :) It looks like the perf difference in the benchmarks is coming from Goldiloader::AssociationPatch#eager_loadable?. We can get all of the performance back by caching the eager loadable checks that are based on model class level information that doesn't change between model instances (i.e. the results of the checks that hit Goldiloader::AssociationInfo
).
benchmark/main.rb
Outdated
@@ -0,0 +1,53 @@ | |||
# frozen_string_literal: true | |||
|
|||
$:.push File.expand_path("lib", __dir__) |
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.
Should this be File.expand_path("../lib", __dir__)
so it points at the project root directory.
0420db3
to
acaf4ff
Compare
All issues fixed including rubocop |
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 great. Thanks for the contribution!
Not sure if there is a way to run benchmark in CI... |
I'd love to have benchmarks running in CI that can automatically flag performance regressions but I haven't seen this work well in oversubscribed CI environments with lots of noisy neighbors. I'm curious if others have seen this work well in other projects. |
My result: