Rails core - May 2005 - mysql_adapter question

Hi everyone,

since I don''t know who wrote that code originally and I am absolutely
puzzled by it, I''ll post the question here. In mysql_adapter.rb, the
function to execute a select statement is written like this: (I''ve
changed the varaible names to make it bit easier to understand)

def select(sql, name = nil)
    result = nil
    @connection.query_with_result = true
    result = execute(sql, name)
    rows = []
    row_template = result.fetch_fields.inject({}) {|t, f| t[f.name] nil; t }
    result.each_hash { |row| rows << row_template.dup.update(row) }
    rows
end

Since each_hash already returns a hash for each row of the result set
already, the last part could have been written like this:

   rows = []
   result.each_hash { |row| rows << row }
   result.free
   rows

I have found this version to be much faster than the original (10%
improvement on a complete request) and to create much less garbage too.
(Why was the free statment missing?)

Does anyone know why this code exists in it''s current version?

-- stefan

> I have found this version to be much faster than the original (10%
> improvement on a complete request) and to create much less garbage  
> too.
> (Why was the free statment missing?)
>
> Does anyone know why this code exists in it''s current version?
I think its just there as a naive first-pass that hasn''t drawn  
attention for revisiting before now. Feel very free to write up a  
patch for this.

And keep on trucking with the performance stuff. I would, however,  
really like to see the performance changes split out into individual  
patches that each document their improvement and can be applied in  
isolation.

Big patches are always scary and require much more investigation  
before committing. Which is also why I''ve been slow to apply your  
great work.
--
David Heinemeier Hansson
http://www.loudthinking.com -- Broadcasting Brain
http://www.basecamphq.com   -- Online project management
http://www.rubyonrails.com  -- Web-application framework

David Heinemeier Hansson wrote:
>> I have found this version to be much faster than the original (10%
>> improvement on a complete request) and to create much less garbage 
too.
>> (Why was the free statment missing?)
>>
>> Does anyone know why this code exists in it''s current version?
>
>
> I think its just there as a naive first-pass that hasn''t drawn  
> attention for revisiting before now. Feel very free to write up a  
> patch for this.
>
OK.
> And keep on trucking with the performance stuff. I would, however,  
> really like to see the performance changes split out into individual  
> patches that each document their improvement and can be applied in  
> isolation.
>
I was just thinking that myself. This way a discussion can be carried 
out individually for each of the patches on the corresponding trac page.
> Big patches are always scary and require much more investigation  
> before committing. Which is also why I''ve been slow to apply your
> great work.
Agreed. And thanks.

-- stefan

PS: I have continued my investigation. Details will follow with each 
patch. Here''s the net result:

configuration 1: r121.latest
configuration 2: r121p.latest.gc100
page                               c1 real   c2 real c1 r/s c2 r/s c1/c2
/empty/index                       6.75525   1.91831  148.0  521.3   3.5
/welcome/index                     6.89044   2.09113  145.1  478.2   3.3
/rezept/index                      4.99573   2.15872  200.2  463.2   2.3
/rezept/myknzlpzl                  4.99592   2.15802  200.2  463.4   2.3
/rezept/show/713                  18.72658   4.92194   53.4  203.2   3.8
/rezept/cat/Hauptspeise           22.67481   5.62603   44.1  177.7   4.0
/rezept/cat/Hauptspeise?page=5    23.03755   5.69712   43.4  175.5   4.0
/rezept/letter/G                  21.66753   5.76091   46.2  173.6   3.8

Empty is is just a single render_text, welcome is action_cached. Both 
empty and welcome create a new session for each request. The large 
improvement on the empty session pages is largely due to the addition of 
a routing cache and the improved session implementation.

Complex pages now render around 4 times as fast, compared to the 
original 0.12.1. I have increased the number of requests to 100 between 
each garbage collection and measured memory consumption (with top) and 
response time using ab. The server process uses somewhat above 30Meg 
(down from 60) and has a maximum response time of 130 ms (when GC kicks in).

More stuff will follow.

> Complex pages now render around 4 times as fast, compared to the  
> original 0.12.1. I have increased the number of requests to 100  
> between each garbage collection and measured memory consumption  
> (with top) and response time using ab. The server process uses  
> somewhat above 30Meg (down from 60) and has a maximum response time  
> of 130 ms (when GC kicks in).
This is very, very cool stuff. I can''t wait to announce all of  
this ;). Please do chop it all up into smaller patches as soon as you  
possibly can.
--
David Heinemeier Hansson
http://www.loudthinking.com -- Broadcasting Brain
http://www.basecamphq.com   -- Online project management
http://www.rubyonrails.com  -- Web-application framework

> def select(sql, name = nil)
>    result = nil
>    @connection.query_with_result = true
>    result = execute(sql, name)
>    rows = []
>    row_template = result.fetch_fields.inject({}) {|t, f| t[f.name] >
nil; t }
>    result.each_hash { |row| rows << row_template.dup.update(row) }
>    rows
> end
Now I remember. result.each_hash won''t return columns with a nil  
value, which then causes a NoMethodError instead of just returning nil.

There might be another, more performant, way of doing this though.  
But it was indeed there for a reason.
--
David Heinemeier Hansson
http://www.loudthinking.com -- Broadcasting Brain
http://www.basecamphq.com   -- Online project management
http://www.rubyonrails.com  -- Web-application framework

> Complex pages now render around 4 times as fast, compared to the  
> original 0.12.1. I have increased the number of requests to 100  
> between each garbage collection and measured memory consumption  
> (with top) and response time using ab. The server process uses  
> somewhat above 30Meg (down from 60) and has a maximum response time  
> of 130 ms (when GC kicks in).
BTW, it would be great to have a reference application to do these  
tests against. Such that we could install it as a sanity check before  
releasing a new version. Did we kill performance?

If there''s a way for you to extract some of this into such an  
application, that would be great.

Alternatively, we could think about using one of the open source  
apps. Like Typo or Hieraki.
--
David Heinemeier Hansson
http://www.loudthinking.com -- Broadcasting Brain
http://www.basecamphq.com   -- Online project management
http://www.rubyonrails.com  -- Web-application framework

David Heinemeier Hansson wrote:
>> def select(sql, name = nil)
>>    result = nil
>>    @connection.query_with_result = true
>>    result = execute(sql, name)
>>    rows = []
>>    row_template = result.fetch_fields.inject({}) {|t, f| t[f.name]
>> nil; t }
>>    result.each_hash { |row| rows << row_template.dup.update(row)
}
>>    rows
>> end
>
>
> Now I remember. result.each_hash won''t return columns with a nil  
> value, which then causes a NoMethodError instead of just returning nil.
Just checked my schema. Don''t have null columns. But I''m still
confused:
if k is not in h.keys, then h[k] would be nil too. So the only way to 
find out is to ask h.hash_key?(k). Can you point me at the place which 
raised the exception?
>
> There might be another, more performant, way of doing this though.  
> But it was indeed there for a reason.
>
10% is a big improvement step. So changing the code which raises the 
exception comes to my mind. Or even changing the MySQL ruby class. I''ll
have a look at it.

But there is also a more radical approach: don''t use hashes at all. 
Instead of returning a hash for a row, create an instance of a row 
class, which has two members: the row values as an array, and a map from 
symbols to column indices. The map can be shared by all rows. Since only 
one hash is created, this may be much faster than the current approach. 
But it would entail a major change for AR and therfore the performance 
impact needs to be measured in an abstract setting first.

-- stefan

David Heinemeier Hansson wrote:
> BTW, it would be great to have a reference application to do these  
> tests against. Such that we could install it as a sanity check before  
> releasing a new version. Did we kill performance?
>
> If there''s a way for you to extract some of this into such an  
> application, that would be great.
This would be very difficult/time consuming for me and I don''t want to 
give my app code away as well. What I can offer, is to test new releases 
on my app, if you drop me a note when you think it is ready, because I 
have to do this anyway.
>
> Alternatively, we could think about using one of the open source  
> apps. Like Typo or Hieraki.
But then everyone would need to set up this app in his development 
environment. I would favor each of the main rails contributors to test 
his own app on the stuff he adds to the code base, before he commits it. 
Then, when the release date approaches, everyone can test the whole 
release for performance. Since every app is different and probably tests 
different aspects of rails, we would get much better data than using 
just one app.

-- stefan

On May 1, 2005, at 6:51 AM, David Heinemeier Hansson wrote:
> BTW, it would be great to have a reference application to do these 
> tests against. Such that we could install it as a sanity check before 
> releasing a new version. Did we kill performance?
> ...
> Alternatively, we could think about using one of the open source apps. 
> Like Typo or Hieraki.
Forgive me if this is na?ve, but could there be a benchmark script or 
Rake task that puts a selected adapter through its paces? If someone 
gets unexpected slowdowns, they could add their case to the benchmarks, 
no?

-- 
Ryan Platte

David Heinemeier Hansson wrote:
> Now I remember. result.each_hash won''t return columns with a nil  
> value, which then causes a NoMethodError instead of just returning nil.
>
I have checked mysql.rb in 
vendor/rails/activerecord/lib/active_record/vendor. It does return 
columns with nil values. Only the C-Version doesn''t. So IMO this is a 
bug in the C-version. I have fixed it on my machine. Here''s is the
patch:
====================================================================---
mysql.c.orig        2005-05-14 14:37:01.639609100 +0200
+++ mysql.c     2005-05-14 14:37:24.795859100 +0200
@@ -806,8 +806,6 @@
     hash = rb_hash_new();
     for (i=0; i<n; i++) {
        VALUE col;
-       if (row[i] == NULL)
-           continue;
        if (with_table == Qnil || with_table == Qfalse)
            col = rb_tainted_str_new2(fields[i].name);
        else {
====================================================================
-- Stefan