-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Fix #12537 performance regression when preloading has_many_through association #19423
Fix #12537 performance regression when preloading has_many_through association #19423
Conversation
@@ -18,7 +18,8 @@ def associated_records_by_owner(preloader) | |||
through_records = owners.map do |owner| | |||
association = owner.association through_reflection.name | |||
|
|||
[owner, Array(association.reader)] | |||
center = association.loaded? ? association.target : association.reader |
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 think we can extract this logic into a method and use it at the both the places.
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 created a method target_records_from_association
and put that line into it.
@yuroyoro Can we add a test case for this? |
I wrote test that asserts the I think this assertion will be failed when changing behavior of preloader on future. |
Please squash your commits. |
aa292fd
to
97a6aba
Compare
Sorry, I squashed |
@sgrif This looks great. Can you take a look? |
For performance, Avoid instantiate CollectionProxy. Fixes rails#12537
97a6aba
to
842f5c2
Compare
@rafaelfranca this is quite old, what is the status? has this already been fixed? |
…_preloading_has_many_through_relation Fix #12537 performance regression when preloading has_many_through association
@@ -89,6 +90,10 @@ def through_scope | |||
|
|||
scope | |||
end | |||
|
|||
def target_records_from_association(association) | |||
association.loaded? ? association.target : association.reader |
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.
@yuroyoro why do you check against loaded?
here? It seems that association is always loaded in this case and all tests remain green when I delete it. Can you add a test that fails without it?
Fix #12537
I profiled the benchmark code that preloads has_many_through association,
and found that most of time spent by calling
CollectionAssociation#reader
method inassociated_records_by_owner
.Here is a result of profiling
associated_records_by_owner
method.CollectionAssociation#reader
creates newCollectionProxy
object if it doesn't exists.But, instantiate
CollectionProxy
is expensive operation.Because creating the
Relation
object by callingAssociation#scope
is expensive,and merging it into
CollectionProxy
is also.c86a32d#commitcomment-5384529
In this case, the association's records are already loaded by the preloader.
It just read records by
Association#target
that returns loaded records,instead of
reader
method.Here are benchmark results.
In the benchmark case "limit 1000 double has_many_through", it still slower than 3.2, but faster then master.
This is a benchmarks code that I used.
https://gist.github.com/yuroyoro/89fae1150e285658db45