Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
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.

This was originally committed in
f176501, and then reverted in
d349490 due to it causing problems in a
real application. This second attempt should solve that.

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...
commit ae934aef4af05f21d231015485cbc5a96df7a4d6 1 parent 96106a1
@jonleighton jonleighton authored
View
32 activerecord/lib/active_record/attribute_methods/read.rb
@@ -32,21 +32,29 @@ 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) }
+ def __temp__#{safe_name}
+ read_attribute(AttrNames::ATTR_#{safe_name}) { |n| missing_attribute(n, caller) }
end
- alias_method '#{attr_name}', :__temp__
- undef_method :__temp__
+ alias_method #{name.inspect}, :__temp__#{safe_name}
+ undef_method :__temp__#{safe_name}
STR
end
View
20 activerecord/lib/active_record/attribute_methods/write.rb
@@ -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
View
7 activerecord/lib/active_record/core.rb
@@ -83,7 +83,12 @@ def initialize_generated_modules
@attribute_methods_mutex = Mutex.new
# force attribute methods to be higher in inheritance hierarchy than other generated methods
- generated_attribute_methods
+ generated_attribute_methods.const_set(:AttrNames, Module.new {
+ def self.const_missing(name)
+ const_set(name, [name.to_s.sub(/ATTR_/, '')].pack('h*').freeze)
+ end
+ })
+
generated_feature_methods
end
Please sign in to comment.
Something went wrong with that request. Please try again.