Permalink
Browse files

Don't allocate new strings in compiled attribute methods

This improves memory and performance without having to use symbols which
present DoS problems. Thanks @headius and @tenderlove for the
suggestion.

Benchmark
---------

require 'active_record'
require 'benchmark/ips'

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database:
':memory:')

class Post < ActiveRecord::Base
  connection.create_table :posts, force: true do |t|
    t.string :name
  end
end

post = Post.create name: 'omg'

Benchmark.ips do |r|
  r.report('Post.new')          { Post.new name: 'omg' }
  r.report('post.name')         { post.name }
  r.report('post.name=')        { post.name = 'omg' }
  r.report('Post.find(1).name') { Post.find(1).name }
end

Before
------

Calculating -------------------------------------
            Post.new      1419 i/100ms
           post.name      7538 i/100ms
          post.name=      3024 i/100ms
   Post.find(1).name       243 i/100ms
-------------------------------------------------
            Post.new    20637.6 (±12.7%) i/s -     102168 in   5.039578s
           post.name  1167897.7 (±18.2%) i/s -    5186144 in   4.983077s
          post.name=    64305.6 (±9.6%) i/s -     317520 in   4.998720s
   Post.find(1).name     2678.8 (±10.8%) i/s -      13365 in   5.051265s

After
-----

Calculating -------------------------------------
            Post.new      1431 i/100ms
           post.name      7790 i/100ms
          post.name=      3181 i/100ms
   Post.find(1).name       245 i/100ms
-------------------------------------------------
            Post.new    21308.8 (±12.2%) i/s -     105894 in   5.053879s
           post.name  1534103.8 (±2.1%) i/s -    7634200 in   4.979405s
          post.name=    67441.0 (±7.5%) i/s -     337186 in   5.037871s
   Post.find(1).name     2681.9 (±10.6%) i/s -      13475 in   5.084511s
  • Loading branch information...
1 parent 9e5f7cc commit f1765019ce9b6292f2264b4601dad5daaffe3a89 @jonleighton jonleighton committed Oct 12, 2012
@@ -35,21 +35,36 @@ def cache_attribute?(attr_name)
protected
- # We want to generate the methods via module_eval rather than define_method,
- # because define_method is slower on dispatch and uses more memory (because it
- # creates a closure).
+ # We want to generate the methods via module_eval rather than
+ # define_method, because define_method is slower on dispatch and
+ # uses more memory (because it creates a closure).
#
- # But sometimes the database might return columns with characters that are not
- # allowed in normal method names (like 'my_column(omg)'. So to work around this
- # we first define with the __temp__ identifier, and then use alias method to
- # rename it to what we want.
- def define_method_attribute(attr_name)
+ # But sometimes the database might return columns with
+ # characters that are not allowed in normal method names (like
+ # 'my_column(omg)'. So to work around this we first define with
+ # the __temp__ identifier, and then use alias method to rename
+ # it to what we want.
+ #
+ # We are also defining a constant to hold the frozen string of
+ # the attribute name. Using a constant means that we do not have
+ # to allocate an object on each call to the attribute method.
+ # Making it frozen means that it doesn't get duped when used to
+ # key the @attributes_cache in read_attribute.
+ def define_method_attribute(name)
+ safe_name = name.unpack('h*').first
generated_attribute_methods.module_eval <<-STR, __FILE__, __LINE__ + 1
- def __temp__
- read_attribute('#{attr_name}') { |n| missing_attribute(n, caller) }
+ module AttrNames
+ unless defined? ATTR_#{safe_name}
+ ATTR_#{safe_name} = #{name.inspect}.freeze
+ end
end
- alias_method '#{attr_name}', :__temp__
- undef_method :__temp__
+
+ def __temp__#{safe_name}
+ read_attribute(AttrNames::ATTR_#{safe_name}) { |n| missing_attribute(n, caller) }
+ end
+
+ alias_method #{name.inspect}, :__temp__#{safe_name}
+ undef_method :__temp__#{safe_name}
STR
end
@@ -9,15 +9,19 @@ module Write
module ClassMethods
protected
- def define_method_attribute=(attr_name)
- if attr_name =~ ActiveModel::AttributeMethods::NAME_COMPILABLE_REGEXP
- generated_attribute_methods.module_eval("def #{attr_name}=(new_value); write_attribute('#{attr_name}', new_value); end", __FILE__, __LINE__)
- else
- generated_attribute_methods.send(:define_method, "#{attr_name}=") do |new_value|
- write_attribute(attr_name, new_value)
- end
+
+ # See define_method_attribute in read.rb for an explanation of
+ # this code.
+ def define_method_attribute=(name)
+ safe_name = name.unpack('h*').first
+ generated_attribute_methods.module_eval <<-STR, __FILE__, __LINE__ + 1
+ def __temp__#{safe_name}=(value)
+ write_attribute(AttrNames::ATTR_#{safe_name}, value)
end
- end
+ alias_method #{(name + '=').inspect}, :__temp__#{safe_name}=
+ undef_method :__temp__#{safe_name}=
+ STR
+ end
end
# Updates the attribute identified by <tt>attr_name</tt> with the

0 comments on commit f176501

Please sign in to comment.