Permalink
Browse files

Removing most of the symbol to proc usage in Active Record

This will hopefully make Active Record run a bit more faster.
  • Loading branch information...
1 parent 79e15f0 commit 433d7a26fe4e68f78a2a606f1c1567c0597fc779 @sikachu sikachu committed with tenderlove Aug 13, 2010
@@ -35,7 +35,7 @@ def initialize(reflection)
through_reflection = reflection.through_reflection
source_reflection_names = reflection.source_reflection_names
source_associations = reflection.through_reflection.klass.reflect_on_all_associations.collect { |a| a.name.inspect }
- super("Could not find the source association(s) #{source_reflection_names.collect(&:inspect).to_sentence(:two_words_connector => ' or ', :last_word_connector => ', or ', :locale => :en)} in model #{through_reflection.klass}. Try 'has_many #{reflection.name.inspect}, :through => #{through_reflection.name.inspect}, :source => <name>'. Is it one of #{source_associations.to_sentence(:two_words_connector => ' or ', :last_word_connector => ', or ', :locale => :en)}?")
+ super("Could not find the source association(s) #{source_reflection_names.collect{ |a| a.inspect }.to_sentence(:two_words_connector => ' or ', :last_word_connector => ', or ', :locale => :en)} in model #{through_reflection.klass}. Try 'has_many #{reflection.name.inspect}, :through => #{through_reflection.name.inspect}, :source => <name>'. Is it one of #{source_associations.to_sentence(:two_words_connector => ' or ', :last_word_connector => ', or ', :locale => :en)}?")
end
end
@@ -1497,17 +1497,17 @@ def collection_reader_method(reflection, association_proxy_class)
association
end
-
+
redefine_method("#{reflection.name.to_s.singularize}_ids") do
if send(reflection.name).loaded? || reflection.options[:finder_sql]
- send(reflection.name).map(&:id)
+ send(reflection.name).map { |r| r.id }
else
if reflection.through_reflection && reflection.source_reflection.belongs_to?
through = reflection.through_reflection
primary_key = reflection.source_reflection.primary_key_name
- send(through.name).select("DISTINCT #{through.quoted_table_name}.#{primary_key}").map!(&:"#{primary_key}")
+ send(through.name).select("DISTINCT #{through.quoted_table_name}.#{primary_key}").map! { |r| r.send(:"#{primary_key}") }
@rymai

rymai Aug 16, 2010

Contributor

Shouldn't :"#{primary_key}" be replaced by primary_key.to_sym here, since it seems faster ?
require 'rubygems'
require 'benchmark'
n = 100_000
primary_key = "id"
Benchmark.bm(20) do |x|
x.report(':"#{primary_key}"') { n.times { :"#{primary_key}" } }
x.report("primary_key.to_sym") { n.times { primary_key.to_sym } }
end

3x faster on REE 1.8.7:
→ ruby -v ruby 1.8.7 (2010-04-19 patchlevel 253) [i686-darwin10.4.0], MBARI 0x6770, Ruby Enterprise Edition 2010.02
user system total real
:"#{primary_key}" 0.070000 0.000000 0.070000 ( 0.077013)
primary_key.to_sym 0.030000 0.000000 0.030000 ( 0.025395)

2.8x faster on Ruby 1.8.7:
→ ruby -v ruby 1.8.7 (2010-06-23 patchlevel 299) [i686-darwin10.4.0]
user system total real
:"#{primary_key}" 0.070000 0.000000 0.070000 ( 0.076286)
primary_key.to_sym 0.030000 0.000000 0.030000 ( 0.026930)

1.75x faster on Ruby 1.9.2:
→ ruby -v ruby 1.9.2dev (2010-07-11 revision 28618) [x86_64-darwin10.4.0]

                        user     system      total        real
:"#{primary_key}"   0.050000   0.000000   0.050000 (  0.049141)
primary_key.to_sym  0.030000   0.000000   0.030000 (  0.028413)
@fxn

fxn Aug 16, 2010

Owner

Doing nothing is even faster :), since #send accepts a string. It was later changed in 22b9bbc

@rymai

rymai Aug 16, 2010

Contributor

Great!

else
- send(reflection.name).select("#{reflection.quoted_table_name}.#{reflection.klass.primary_key}").except(:includes).map!(&:id)
+ send(reflection.name).select("#{reflection.quoted_table_name}.#{reflection.klass.primary_key}").except(:includes).map! { |r| r.id }
end
end
end
@@ -1529,7 +1529,7 @@ def collection_accessor_methods(reflection, association_proxy_class, writer = tr
pk_column = reflection.primary_key_column
ids = (new_value || []).reject { |nid| nid.blank? }
ids.map!{ |i| pk_column.type_cast(i) }
- send("#{reflection.name}=", reflection.klass.find(ids).index_by(&:id).values_at(*ids))
+ send("#{reflection.name}=", reflection.klass.find(ids).index_by{ |r| r.id }.values_at(*ids))
end
end
end
@@ -127,7 +127,7 @@ def create_record(attributes, &block)
def record_timestamp_columns(record)
if record.record_timestamps
- record.send(:all_timestamp_attributes).map(&:to_s)
+ record.send(:all_timestamp_attributes).map { |x| x.to_s }
else
[]
end
@@ -231,7 +231,7 @@ def associated_records_to_validate_or_save(association, new_record, autosave)
def nested_records_changed_for_autosave?
self.class.reflect_on_all_autosave_associations.any? do |reflection|
association = association_instance_get(reflection.name)
- association && Array.wrap(association.target).any?(&:changed_for_autosave?)
+ association && Array.wrap(association.target).any? { |a| a.changed_for_autosave? }
end
end
@@ -519,7 +519,7 @@ def count_by_sql(sql)
# Attributes listed as readonly will be used to create a new record but update operations will
# ignore these fields.
def attr_readonly(*attributes)
- write_inheritable_attribute(:attr_readonly, Set.new(attributes.map(&:to_s)) + (readonly_attributes || []))
+ write_inheritable_attribute(:attr_readonly, Set.new(attributes.map { |a| a.to_s }) + (readonly_attributes || []))
end
# Returns an array of all the attributes that have been specified as readonly.
@@ -1286,7 +1286,7 @@ def sanitize_sql_hash_for_conditions(attrs, default_table_name = self.table_name
table = Arel::Table.new(self.table_name, :engine => arel_engine, :as => default_table_name)
builder = PredicateBuilder.new(arel_engine)
- builder.build_from_hash(attrs, table).map(&:to_sql).join(' AND ')
+ builder.build_from_hash(attrs, table).map{ |b| b.to_sql }.join(' AND ')
end
alias_method :sanitize_sql_hash, :sanitize_sql_hash_for_conditions
@@ -1737,7 +1737,7 @@ def execute_callstack_for_multiparameter_attributes(callstack)
klass = (self.class.reflect_on_aggregation(name.to_sym) || column_for_attribute(name)).klass
# in order to allow a date to be set without a year, we must keep the empty values.
# Otherwise, we wouldn't be able to distinguish it from a date with an empty day.
- values = values_with_empty_parameters.reject(&:nil?)
+ values = values_with_empty_parameters.reject { |v| v.nil? }
if values.empty?
send(name + "=", nil)
@@ -528,7 +528,7 @@ def references(*args)
# concatenated together. This string can then be prepended and appended to
# to generate the final SQL to create the table.
def to_sql
- @columns.map(&:to_sql) * ', '
+ @columns.map { |c| c.to_sql } * ', '
end
private
@@ -450,7 +450,7 @@ def assume_migrated_upto_version(version, migrations_path = ActiveRecord::Migrat
version = version.to_i
sm_table = quote_table_name(ActiveRecord::Migrator.schema_migrations_table_name)
- migrated = select_values("SELECT version FROM #{sm_table}").map(&:to_i)
+ migrated = select_values("SELECT version FROM #{sm_table}").map { |v| v.to_i }
versions = Dir["#{migrations_path}/[0-9]*_*.rb"].map do |filename|
filename.split('/').last.split('_').first.to_i
end
@@ -879,7 +879,7 @@ def distinct(columns, order_by) #:nodoc:
# Construct a clean list of column names from the ORDER BY clause, removing
# any ASC/DESC modifiers
order_columns = order_by.split(',').collect { |s| s.split.first }
- order_columns.delete_if(&:blank?)
+ order_columns.delete_if { |c| c.blank? }
order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
# Return a DISTINCT ON() clause that's distinct on the columns we want but includes
@@ -40,11 +40,11 @@ class Version
include Comparable
def initialize(version_string)
- @version = version_string.split('.').map(&:to_i)
+ @version = version_string.split('.').map { |v| v.to_i }
end
def <=>(version_string)
- @version <=> version_string.split('.').map(&:to_i)
+ @version <=> version_string.split('.').map { |v| v.to_i }
end
end
@@ -345,7 +345,7 @@ def copy_table_indexes(from, to, rename = {}) #:nodoc:
name = name[5..-1]
end
- to_column_names = columns(to).map(&:name)
+ to_column_names = columns(to).map { |c| c.name }
columns = index.columns.map {|c| rename[c] || c }.select do |column|
to_column_names.include?(column)
end
@@ -690,7 +690,7 @@ def inheritance_column_name
end
def column_names
- @column_names ||= @connection.columns(@table_name).collect(&:name)
+ @column_names ||= @connection.columns(@table_name).collect { |c| c.name }
end
def read_fixture_files
@@ -908,7 +908,7 @@ def setup_fixture_accessors(table_names = nil)
def uses_transaction(*methods)
@uses_transaction = [] unless defined?(@uses_transaction)
- @uses_transaction.concat methods.map(&:to_s)
+ @uses_transaction.concat methods.map { |m| m.to_s }
end
def uses_transaction?(method)
@@ -374,7 +374,7 @@ def connection
end
def method_missing(method, *arguments, &block)
- arg_list = arguments.map(&:inspect) * ', '
+ arg_list = arguments.map{ |a| a.inspect } * ', '
say_with_time "#{method}(#{arg_list})" do
unless arguments.empty? || method == :execute
@@ -451,7 +451,7 @@ def schema_migrations_table_name
def get_all_versions
table = Arel::Table.new(schema_migrations_table_name)
- Base.connection.select_values(table.project(table['version']).to_sql).map(&:to_i).sort
+ Base.connection.select_values(table.project(table['version']).to_sql).map{ |v| v.to_i }.sort
end
def current_version
@@ -569,7 +569,7 @@ def migrations
klasses << migration
end
- migrations = migrations.sort_by(&:version)
+ migrations = migrations.sort_by { |m| m.version }
down? ? migrations.reverse : migrations
end
end
@@ -122,7 +122,7 @@ def add_observer!(klass)
end
def define_callbacks(klass)
- existing_methods = klass.instance_methods.map(&:to_sym)
+ existing_methods = klass.instance_methods.map { |m| m.to_sym }
observer = self
observer_name = observer.class.name.underscore.gsub('/', '__')
@@ -375,15 +375,15 @@ def method_missing(method, *args, &block)
def references_eager_loaded_tables?
# always convert table names to downcase as in Oracle quoted table names are in uppercase
- joined_tables = (tables_in_string(arel.joins(arel)) + [table.name, table.table_alias]).compact.map(&:downcase).uniq
+ joined_tables = (tables_in_string(arel.joins(arel)) + [table.name, table.table_alias]).compact.map{ |t| t.downcase }.uniq
(tables_in_string(to_sql) - joined_tables).any?
end
def tables_in_string(string)
return [] if string.blank?
# always convert table names to downcase as in Oracle quoted table names are in uppercase
# ignore raw_sql_ that is used by Oracle adapter as alias for limit/offset subqueries
- string.scan(/([a-zA-Z_][\.\w]+).?\./).flatten.map(&:downcase).uniq - ['raw_sql_']
+ string.scan(/([a-zA-Z_][\.\w]+).?\./).flatten.map{ |s| s.downcase }.uniq - ['raw_sql_']
end
end
@@ -348,7 +348,7 @@ def column_aliases(join_dependency)
end
def using_limitable_reflections?(reflections)
- reflections.none?(&:collection?)
+ reflections.none? { |r| r.collection? }
end
end
@@ -123,7 +123,7 @@ def table(table, stream)
end.compact
# find all migration keys used in this table
- keys = [:name, :limit, :precision, :scale, :default, :null] & column_specs.map(&:keys).flatten
+ keys = [:name, :limit, :precision, :scale, :default, :null] & column_specs.map{ |k| k.keys }.flatten
# figure out the lengths for each column based on above keys
lengths = keys.map{ |key| column_specs.map{ |spec| spec[key] ? spec[key].length + 2 : 0 }.max }
@@ -21,7 +21,7 @@ def assert_sql(*patterns_to_match)
patterns_to_match.each do |pattern|
failed_patterns << pattern unless $queries_executed.any?{ |sql| pattern === sql }
end
- assert failed_patterns.empty?, "Query pattern(s) #{failed_patterns.map(&:inspect).join(', ')} not found.#{$queries_executed.size == 0 ? '' : "\nQueries:\n#{$queries_executed.join("\n")}"}"
+ assert failed_patterns.empty?, "Query pattern(s) #{failed_patterns.map{ |p| p.inspect }.join(', ')} not found.#{$queries_executed.size == 0 ? '' : "\nQueries:\n#{$queries_executed.join("\n")}"}"
end
def assert_queries(num = 1)

19 comments on commit 433d7a2

"Hopefully" - what did the benchmarks say?

Owner

tenderlove replied Aug 14, 2010

AR tests run 10% faster on my machine with sqlite3.

great :)

I wondered why this is faster and found this blog post which explains it: http://m.onkey.org/2007/6/30/let-s-start-with-wtf
I am also glad that I can forget this ampersand notation again.

Contributor

norman replied Aug 14, 2010

@RainerBlessing

That article was benchmarking performance of Active Support's emulated symbol_to_proc on 1.8.6. As far as I can tell, now that this is part of Ruby 1.8.7+, performance at this point is usually pretty similar.

http://gist.github.com/524333

I'm not sure that symbol to proc was the only culprit above, it may have been a combination of things. Perhaps I've misunderstood something, I don't know. Either way though, 10% faster FTW. :)

Owner

tenderlove replied Aug 14, 2010

@norman try this benchmark:

require 'benchmark'

10_000.step(10_000_000, 10_000) do |n|
  puts "N: #{n}"
  puts "######################"
  Benchmark.bm(11) do |x|
    x.report("block") do
      n.times do
        [1,2,3,4,5].map { |v| v.to_s }
      end
    end

    x.report("symbol2proc") do
      n.times do
        [1,2,3,4,5].map(&:to_s)
      end
    end
  end
end

symbol to proc very quickly becomes slower than the block form on my system which is: ruby 1.8.7 (2009-06-08 patchlevel 173) [universal-darwin10.0]

Contributor

stevegraham replied Aug 14, 2010

+1 to @norman's point

This dislike of Symbol#to_proc is misguided. Although the difference is practically neglible, Symbol#to_proc is actually faster than the block syntax in Ruby 1.9.2, e.g.

require "benchmark"

Benchmark.bmbm do |ex|
  ex.report('block') { 1000000.times { [1,2,3,4,5,6,7,8,9,0].map { |i| i.to_s } } }
  ex.report('Symbol#to_proc') { 1000000.times { [1,2,3,4,5,6,7,8,9,0].map &:to_s } }
end

Results:

Rehearsal --------------------------------------------------
block            3.880000   0.010000   3.890000 (  3.888430)
Symbol#to_proc   3.260000   0.000000   3.260000 (  3.262747)
----------------------------------------- total: 7.150000sec

                     user     system      total        real
block            3.890000   0.020000   3.910000 (  3.905643)
Symbol#to_proc   3.250000   0.010000   3.260000 (  3.249109)
Contributor

stevegraham replied Aug 14, 2010

@tenderlove: I tried your benchmark on 1.9.2 and Symbol#to_proc is always faster than the block syntax, e.g. excerpt:

N: 880000
######################
                 user     system      total        real
block        2.250000   0.010000   2.260000 (  2.298891)
symbol2proc  1.910000   0.000000   1.910000 (  1.928648)

My view is those using the 1.9.x branch should not have to pay a performance cost and forgo beautiful code because Symbol#to_proc is slightly slower on 1.8.7.

The (slight) slowness of Symbol#to_proc compared to the block syntax on 1.8.7 is a cost of staying on 1.8.7 and should be an incentive to upgrade to 1.9.x. It is unfair 1.9.x users have to bear this cost.

This commit should be reverted.

S

Owner

tenderlove replied Aug 14, 2010

@stevegraham it depends on the version of Ruby you're using. If you're using 1.8.7, it is not misguided. Please run my benchmark with 1.8.7. When 1.8.x support is dropped, it will be good to switch to Symbol#to_proc. But in the mean time, I'm happy the tests are 10% faster.

Contributor

stevegraham replied Aug 14, 2010

@tenderlove I hear what you are saying but 1.8.7 is still supported using Symbol#to_proc, the performance characteristics are just suboptimal. The language has advanced both feature wise and performance wise since 1.8.7. We should be incentivising people to upgrade to 1.9.x in order for them to reap these benefits and to facilitate the onward march of Ruby development.

Correct me if I am wrong here, but my understanding is although 1.8.7 is supported 1.9.2 is preferred for Rails 3.0. So in line with this preference I do not see the logic in using the block syntax. In Ruby 1.9.2 Symbol#to_proc is prettier and it is faster, ergo that is what should be used.

S

Contributor

norman replied Aug 14, 2010

@tenderlove

Thanks for the clarification. I get the same results as you on ruby 1.8.7 (2010-06-23 patchlevel 299) [i686-darwin10.4.0].

@stevengraham

I also like #to_proc better, but if 1.8.7 is to be supported, it seems like a pretty small price to pay for 10% speed improvement.

My earlier point was because I wasn't myself able to see the difference, and I also didn't want @RainerBlessing to "just forget" about it. :)

Owner

tenderlove replied Aug 14, 2010

@norman I agree. We shouldn't forget about the &:sym syntax. We should just use it in places where performance doesn't matter. In AR, performance matters.

@stevengraham At N = 880000, 1.8.7 is over 2x slower when using symbol to proc. Should we just tell people using 1.8.7 to upgrade? Is that fair to them? The speed difference in 1.9 is minimal, but in 1.8, it's massive. We need to strike a balance.

Contributor

stevegraham replied Aug 14, 2010

@tenderlove The difference, as you said that this commit has is a 10% reduction in execution time of the test suite when using sqlite. So keeping Symbol#to_proc does not make things "2x slower", using your benchmark as an example is reductio ad absurdum. Rails 3 is a major milestone where things move forward, so no I don't think this is unfair on those using 1.8.7. The same reasons that make people unable or unwilling to move to 1.9.x are likely the same reasons that will make them unable or unwilling to move to Rails 3.

I don't think this commit is a bad idea in general. To the contrary, I believe that this commit is a great idea for the Rails 2.3.x branch.

S

I have to agree with @stevegraham and @norman. This commit is slowing down AR3 on the newest version of Ruby, so that 1.8 runs a bit faster? That's just backwards. Ruby 1.9.2 is a major milestone, like Rails 3, and opportunities like this are reasons for people to upgrade. We've been running 1.9 in production since it stabilized, and are counting the days to jump to 1.9.2.

@nateware look at the benchmarks. It's not slowing down AR3 so that Ruby 1.8 runs a bit faster. It is very minimally slowing down AR3 on Ruby 1.9 so that 1.8 runs a lot faster. It's a fair trade.

Contributor

norman replied Aug 16, 2010

@nateware

Actually I agree with @tenderlove that this commit should be applied to the 3.0 branch. Let's please not overstate the consequences this commit has on 1.9.2. We're talking about a trivial slowdown on Ruby 1.9.2, and a major improvement on 1.8.7. My initial comment was because I didn't understand the issue as well as @sikachu and @tenderlove do, and thought that symbol_to_proc was just as fast on 1.8.7, which is not correct.

Supporting multiple versions means occasionally having to make tradeoffs. Ruby 1.8.7 is going to remain a very important platform for a while longer, and during that time it makes good sense for it to be well supported, particularly when doing so implies very little impact on the support for 1.9.2, as is the case here.

Rails 3 already drops support for 1.8.6 in favor of the improvements offered by 1.8.7. Let's not get too greedy. :)

This is a bit orthogonal, but I remain surprised at the number of people who haven't upgraded to 1.9.1. My only conclusion is many people must read blogs and take them as gospel, rather than testing their own apps. When we ran our benchmarks, 1.9.1 offered significant performance improvements for us, and minimal incompatibilities (and we have hundreds of models). I'm forced to conclude many people are simply afraid to upgrade because a small number of people have blogged about a handful of issues. So we end up with compromises like this, which is too bad.

Contributor

norman replied Aug 16, 2010

There were some pretty big problems running Rails 3 on 1.9.1, including segfaults. Rails 3 and Ruby 1.9.2 promise to be a much more stable combination, and I would expect adoption to soar once both are released.

I was referring to Rails 2.3.x, which we've been running with Ruby 1.9.1 for a long while. But your points about Rails 3 are completely valid.

Please sign in to comment.