Rails core - Feb 2008 - Review Request for Significant Performance Improvement in ActiveRecord

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.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 25 Feb 2008, at 08:12, dasil003 wrote:
>
> 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
> 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.
> >

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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.

On Feb 25, 3:49 am, Frederick Cheung <frederick.che...@gmail.com>
wrote:
> On 25 Feb 2008, at 08:12, dasil003 wrote:
>
>
>
> > 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 withhttp://dev.rubyonrails.org/ticket/9640in  
> trunk ?
>
> Fred
>
> >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_e...
>
> > 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.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 27 Feb 2008, at 08:01, dasil003 wrote:
>
> 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.

> On Feb 25, 3:49 am, Frederick Cheung <frederick.che...@gmail.com>
> wrote:
>> On 25 Feb 2008, at 08:12, dasil003 wrote:
>>
>>
>>
>>> 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 withhttp://dev.rubyonrails.org/ticket/9640in
>> trunk ?
>>
>> Fred
>>
>>> 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_e
>>> ...
>>
>>> 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.
> >

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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.

On Feb 27, 1:55 am, Frederick Cheung <frederick.che...@gmail.com>
wrote:
> On 27 Feb 2008, at 08:01, dasil003 wrote:
>
>
>
> > 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.
>
> > On Feb 25, 3:49 am, Frederick Cheung
<frederick.che...@gmail.com>
> > wrote:
> >> On 25 Feb 2008, at 08:12, dasil003 wrote:
>
> >>> 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
withhttp://dev.rubyonrails.org/ticket/9640in
> >> trunk ?
>
> >> Fred
>
> >>>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_e
> >>> ...
>
> >>> 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.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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

On Feb 29, 6:09 pm, dasil003 <gabrie...@gmail.com>
wrote:
> 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.
>
> On Feb 27, 1:55 am, Frederick Cheung <frederick.che...@gmail.com>
> wrote:
>
> > On 27 Feb 2008, at 08:01, dasil003 wrote:
>
> > > 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.
>
> > > On Feb 25, 3:49 am, Frederick Cheung
<frederick.che...@gmail.com>
> > > wrote:
> > >> On 25 Feb 2008, at 08:12, dasil003 wrote:
>
> > >>> 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
withhttp://dev.rubyonrails.org/ticket/9640in
> > >> trunk ?
>
> > >> Fred
>
> > >>>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_e
> > >>> ...
>
> > >>> 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.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

Marvelous,  Applied!

On Mon, Mar 3, 2008 at 11:55 AM, dasil003 <gabriel.d@gmail.com>
wrote:
>
>  Patch is updated with Koz''s style suggestions and is now applying
>  cleanly to trunk (in case anyone tried before and was discouraged).
>
>
>
>  On Feb 29, 6:09 pm, dasil003 <gabrie...@gmail.com> wrote:
>  > 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.
>  >
>  > On Feb 27, 1:55 am, Frederick Cheung
<frederick.che...@gmail.com>
>  > wrote:
>  >
>  > > On 27 Feb 2008, at 08:01, dasil003 wrote:
>  >
>  > > > 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.
>  >
>  > > > On Feb 25, 3:49 am, Frederick Cheung
<frederick.che...@gmail.com>
>  > > > wrote:
>  > > >> On 25 Feb 2008, at 08:12, dasil003 wrote:
>  >
>  > > >>> 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
withhttp://dev.rubyonrails.org/ticket/9640in
>  > > >> trunk ?
>  >
>  > > >> Fred
>  >
>  > > >>>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_e
>  > > >>> ...
>  >
>  > > >>> 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.
>  >
>


-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---