[PPATCH] performance patch for AR::Base

This patch catches a lot of the low-hanging fruit in AR::Base.

In id and read_attribute you can use define_method rather than class_eval, just for cleanliness.

In read_attribute, use attr_name.to_sym rather than attr_name.intern. It works for both String and Symbol.

In attributes_with_quotes, you can still use inject and use key,value rather than pair:

attributes.inject do |attrs_quoted, (key, value)|

I think the attributes_from_column_definition fix may be included in another patch. It's been a long time coming, in any case.

Thanks Stefan.

In id and read_attribute you can use define_method rather than class_eval, just for cleanliness.

No, you can't, because cast_code is a string.

In attributes_with_quotes, you can still use inject and use key,value rather than pair

Yes I could, but I liked my version better.

Thanks for the to_sym tip.

Another point for optimization could be to use blocks instead of returning and iterating through arrays in methods database adapters and AR:Base.

I don't know about the blocks, because blocks involve creating closures and that is not exactly cheap either. I would not attempt it without careful profiling.

I'd prefer to try the following: instead of creating a attributes hash to hold the column values, I would use a row class with a pointer to the field information, and the column values as an array. Then the column value accessors could be optimized to use integer indices instead of strings. The field information class could be defined based on the tables involved and the fields named in the select clause. There would be only a single instance for each field information class, so that on a second query we could simply retrieve the pointer. For records coming into existence via create in AR class X, the field information class for "SELECT * from X.table_name" would be used.

This would of course be a major rewrite of AR internals, but I have the feeling it could give a big performance boost, because field access will (hopefully) be faster and less data gets created on the fly, which will improve GC times too.

BTW, this would also fix http://dev.rubyonrails.org/ticket/240

What do you think?

bug fix and additional test for it

Stefan, I love the idea of using a Row class -- I have this agenda to find an unobtrusive way to be able to switch to simple in-memory during tests, and in mulling the issues involved, wished for this very thing.

Grossly speaking, AR::Base has two major responsibilities, and separating the higher-level responsibilities from the lower-level interaction with the database adapter would add a lot of flexibility. I posted about this in the discussion at http://groups.yahoo.com/group/testdrivendevelopment/message/11705

Please contact me to solicit my help if you want to work on this.

[OT]

Ryan, regarding your message to the Yahoo group: turn on transactional fixtures. They populate your db with your fixtures before the test run then rollback after each test. To enable by default, add

Test::Unit::TestCase.use_transactional_fixtures = true

to test/test_helper.rb.

To disable for a specific test, add

self.use_transaction_fixtures = false

to the top of the class definition.

It isn't mock objects for sure, but it is far faster than deleting/inserting fixtures per-test.

Thanks a ton, Jeremy! I'd read about transactional fixtures but forgotten. That does ease the pain quite a bit.

I still want to do something about separating out the adapter-specific stuff if I can sometime, though.

Applied in [2482], [2483], [2509] and [2511].

IMHO, this was closed a bit early. Several important optimizations from this patch have been left out and I would like to know why.

The difference between current trunk and all optimizations applied ranges from 12 to 25% on uncached pages in my app.

$ pc /d/perfdata/10-10.all.trunk.txt /d/perfdata/10-10.all.ptrunk.txt
perf data file 1: 10-10.all.trunk
  requests=1000, options=-bm=all -mysql_session -fast_routes -lib=trunk

perf data file 2: 10-10.all.ptrunk
  requests=1000, options=-bm=all -mysql_session -fast_routes -lib=ptrunk

page                               c1 real   c2 real    r/s    r/s   ms/r   ms/r  c1/c2
/empty/index                       1.49466   1.48035  669.0  675.5   1.49   1.48   1.01
/welcome/index                     1.65082   1.64067  605.8  609.5   1.65   1.64   1.01
/rezept/index                      1.52628   1.51083  655.2  661.9   1.53   1.51   1.01
/rezept/myknzlpzl                  1.52055   1.49778  657.7  667.7   1.52   1.50   1.02
/rezept/show/713                   4.48284   3.58257  223.1  279.1   4.48   3.58   1.25
/rezept/cat/Hauptspeise            5.23591   4.55861  191.0  219.4   5.24   4.56   1.15
/rezept/cat/Hauptspeise?page=5     5.34135   4.66202  187.2  214.5   5.34   4.66   1.15
/rezept/letter/G                   5.14834   4.59844  194.2  217.5   5.15   4.60   1.12

Thanks for all the work you've done toward improving Rails' performance.

Not all of the above patch was included when I closed the ticket, it's true. This was a big patch that did not apply cleanly to trunk and included at least 9 categorically discrete modifications. I manually applied everything in the patch. What you don't find in trunk currently is not there because it caused many tests to fail.

Here is a list of things that didn't get committed because it did not work against HEAD:

• The reset_table_name and table_name refactoring in AR::Base
• null_values_in_each_hash and select_all in the mysql adapter
• moving dozens of utility methods out of the instance and into the class/module needs to be done in a more consistant way
• passing the connection explicitly to all those methods conflicts with the approach in [2162] which David would like to see in 1.0

Please resubmit against HEAD (preferably in separate patches) with all tests passing in a new ticket.

Marcel, when I submitted this patch it cleanly applied and it passed all tests.

David knew that, and intentionally broke it when refactoring some AR stuff. He assured me that he (or somone else) would apply each change individually anyway and that I need not worry about the breakage.

Now I'm being asked to rewrite it again. I'm not sure if I want to do that. At least not without assurance that the new patches will be applied/worked on in a timely manner.

I'm leaving the reopend until we have resolved the open issues (possibly by opeing ne tickets).

I'm closing this, as the performance goals have been achieved through several smaller patches. However, for multithreaded enviroments (e.g. Webrick), it would still be very beneficial to move the quoting methods from instance to class level on the adapters.

Here's the latest perf data:

perf data file 1: 10-11.all.ptrunk
  requests=1000, options=-bm=all -mysql_session -fast_routes -lib=ptrunk

perf data file 2: 10-11.all.trunk
  requests=1000, options=-bm=all -mysql_session -fast_routes -lib=trunk

page                               c1 real   c2 real    r/s    r/s   ms/r   ms/r  c1/c2
/empty/index                       1.46928   1.48286  680.6  674.4   1.47   1.48   0.99
/welcome/index                     1.62987   1.63549  613.5  611.4   1.63   1.64   1.00
/rezept/index                      1.50227   1.49449  665.7  669.1   1.50   1.49   1.01
/rezept/myknzlpzl                  1.48972   1.48886  671.3  671.7   1.49   1.49   1.00
/rezept/show/713                   3.56071   3.59163  280.8  278.4   3.56   3.59   0.99
/rezept/cat/Hauptspeise            4.54186   4.57079  220.2  218.8   4.54   4.57   0.99
/rezept/cat/Hauptspeise?page=5     4.64643   4.66885  215.2  214.2   4.65   4.67   1.00
/rezept/letter/G                   4.59211   4.62213  217.8  216.4   4.59   4.62   0.99