Do not instantiate intermediate AR objects when eager loading. #8403

Merged
merged 1 commit into from Dec 4, 2012

Projects

None yet

3 participants

Owner
senny commented Dec 2, 2012

There are eager-loading queries that Active Record performs in two steps.

  1. Fetch the ID of the relevant records
  2. Perform the actual select to instantiate AR objects

Currently step one also instantiated intermediate ActiveRecord objects just to pick the ID from them. This changes fetches the IDs using select_all, which does not instante objects.

The intermediate objects were a problem because the after_find callback was triggered twice. Also I think it does not make sense to instantiate objects that we don't actually need.

This is a fix for #3313

I'm not sure if my test-case is very helpful. It was the best I could come up with to illustrate the problem.

@senny senny and 1 other commented on an outdated diff Dec 2, 2012
...verecord/lib/active_record/relation/finder_methods.rb
@@ -225,9 +225,11 @@ def construct_limited_ids_condition(relation)
orders = relation.order_values.map { |val| val.presence }.compact
values = @klass.connection.distinct("#{quoted_table_name}.#{primary_key}", orders)
- relation = relation.dup
+ relation = relation.dup.select(values)
+
+ id_rows = @klass.connection.select_all(relation.arel, nil, relation.bind_values)
Owner
senny commented Dec 2, 2012

I think we should also backport this change to 3-2-stable.

@rafaelfranca what do you think?

Owner

Seems good.

@jonleighton @tenderlove what do you think?

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Dec 3, 2012
activerecord/CHANGELOG.md
@@ -1,5 +1,11 @@
## Rails 4.0.0 (unreleased) ##
+* Do not instantiate intermediate Active Record objects when eager loading.
+ These records caused `after_find` to be fiered more than necessary.
Owner
senny commented Dec 4, 2012

@rafaelfranca I used "SQL" as the name. PR is updated and rebased.

@rafaelfranca rafaelfranca merged commit a9dc446 into rails:master Dec 4, 2012
Owner

Thank you

@senny senny added a commit to senny/rails that referenced this pull request Dec 4, 2012
@senny @senny senny + senny backport #8403, no intermediate AR objects when eager loading.
Closes #3313

Conflicts:

	activerecord/CHANGELOG.md
	activerecord/test/models/developer.rb
1b96176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment