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

Optimize association eager loading for a single model #80

Merged
merged 11 commits into from
Dec 10, 2018

Conversation

jturkel
Copy link
Member

@jturkel jturkel commented Dec 7, 2018

This change optimizes automatic eager loading when there's only a single model in the auto include context by just doing regular Rails association loading. This bypasses a bunch of extra work that the Rails preloader does. In benchmarks this reduces object allocations by 5% but does not result in a statistically significant throughput change (even though it was faster by about 10% in all of the experiments I ran):

Goldiloader: Single model's association
                          2.287k (± 8.5%) i/s -     22.848k in  10.069578s
Goldiloader: Single model's association optimized
                          2.536k (± 7.9%) i/s -     25.194k in  10.004004s

Comparison:
Goldiloader: Single model's association optimized:     2535.6 i/s
Goldiloader: Single model's association:     2287.4 i/s - same-ish: difference falls within error

I added a new benchmark as part of this change so I factored out some commonality between the benchmarks and added a script for running all benchmarks.

@fgarces - you're prime

@jturkel jturkel requested a review from fgarces December 7, 2018 19:57
Copy link
Member

@fgarces fgarces left a comment

Choose a reason for hiding this comment

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

🚢

require_relative 'performance_helper'

ForkingBenchmark.ips do |x|
x.time = 5
Copy link
Member

Choose a reason for hiding this comment

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

is there any reason for 5 and 2 or just because they sounded fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

They were copied from the benchmark that landed in #77. Looks like these are the defaults for benchmark-ips though so I'll remove them.

@@ -2,6 +2,7 @@

### 3.0.3
- Optimize association eager loadable checks by caching information on the association's reflection.
- Optimize association eager loading if we're only eager loading associations for a single model.
Copy link
Member

Choose a reason for hiding this comment

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

I assume you havent released the gem, otherwise this should go into 3.0.4.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. I haven't released 3.0.3 yet.

@jturkel jturkel merged commit 46c5107 into master Dec 10, 2018
@jturkel jturkel deleted the feature/optimizing-singles branch December 10, 2018 18:54
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.

None yet

2 participants