Review Request for Significant Performance Improvement in ActiveRecord

rubyonrails-core
1

I'm going through my old tickets, and I found an outstanding ticket that I've put many hours into, and is absolutely critical for a production project that I'm now updating to Rails 2.0.2. The ticket:

http://dev.rubyonrails.org/ticket/9560

This comes on the tail of:

http://dev.rubyonrails.org/ticket/9497

Which I believed to be the superior improvement due to a very carefully considered thesis elaborated at:

http://darwinweb.net/articles/66-optimizing_and_simplifying_limited_eager_loading_in_activerecord

I'd appreciate if I could get some reviewers for this patch, since my project's performance is simply not viable without this patch. Despite the weirdness of the actual diff, the change is mostly piggybacking on top of logic that's already there.

2

I'm going through my old tickets, and I found an outstanding ticket that I've put many hours into, and is absolutely critical for a production project that I'm now updating to Rails 2.0.2. The ticket:

Is still still relevant with http://dev.rubyonrails.org/ticket/9640 in
trunk ?

Fred

3

Awesome work there, I will definitely be giving that a run for the money. 9640 solves a much more fundamental problem with eager loading, but I think there's still a place for my patch. Specifically because the id-fetching pre-query operates over a large number of rows, and hence joins there are particularly expensive. My patch eliminates unnecessary joins in that query, but leaves them in the second query which is already much much faster because the rows are limited by the 'id IN (1,2,3,4,...) clause.

4

Awesome work there, I will definitely be giving that a run for the money. 9640 solves a much more fundamental problem with eager loading, but I think there's still a place for my patch. Specifically because the id-fetching pre-query operates over a large number of rows, and hence joins there are particularly expensive. My patch eliminates unnecessary joins in that query, but leaves them in the second query which is already much much faster because the rows are limited by the 'id IN (1,2,3,4,...) clause.

Ah yes, there is still the case where it falls back to the old code. I've quite often written (though less so since 9640) ids = Foo.find(...).map &:id foos = Foo.find(ids, :include => [...]) and it would be nice to eliminate that repetition.

5

I'm renewing my request for reviewers here. Guys, this patch should be a no brainer. It solves a performance issue in a non-destructive way. It should take you 10 mins to install it in your app, run your tests and give the +1.

6

Patch is updated with Koz's style suggestions and is now applying cleanly to trunk (in case anyone tried before and was discouraged).

7

Marvelous, Applied!