Skip to content

Commit

Permalink
Cheaper attribute reads and respond_to?. Add underscore-prefixed meth…
Browse files Browse the repository at this point in the history 
…od aliased to the attribute name so it can be overridden but still called internally.
  • Loading branch information
@jeremy @tenderlove
jeremy authored and tenderlove committed Mar 28, 2011
1 parent 004fc1c commit 86acbf1
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 5 deletions.
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/attribute_methods.rb
View file
Expand Up @@ -48,7 +48,7 @@ def method_missing(method_id, *args, &block)
end

def respond_to?(*args)
self.class.define_attribute_methods
self.class.define_attribute_methods unless self.class.attribute_methods_generated?
super
end

Expand Down
8 changes: 6 additions & 2 deletions activerecord/lib/active_record/attribute_methods/read.rb
View file
Expand Up @@ -77,6 +77,12 @@ def define_read_method(symbol, attr_name, column)
# Returns the value of the attribute identified by <tt>attr_name</tt> after it has been typecast (for example,
# "2004-12-12" in a data column is cast to a date object, like Date.new(2004, 12, 12)).
def read_attribute(attr_name)
send "_#{attr_name}"
rescue NoMethodError
_read_attribute attr_name
end

def _read_attribute(attr_name)
attr_name = attr_name.to_s
attr_name = self.class.primary_key if attr_name == 'id'
value = @attributes[attr_name]
Expand All @@ -90,8 +96,6 @@ def read_attribute(attr_name)
else
value
end
else
nil
end
end

Expand Down
5 changes: 3 additions & 2 deletions activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
View file
Expand Up @@ -19,12 +19,13 @@ module ClassMethods
def define_method_attribute(attr_name)
if create_time_zone_conversion_attribute?(attr_name, columns_hash[attr_name])
method_body, line = <<-EOV, __LINE__ + 1
def #{attr_name}(reload = false)
def _#{attr_name}(reload = false)
cached = @attributes_cache['#{attr_name}']
return cached if cached && !reload
time = read_attribute('#{attr_name}')
time = _read_attribute('#{attr_name}')
@attributes_cache['#{attr_name}'] = time.acts_like?(:time) ? time.in_time_zone : time
end
alias #{attr_name} _#{attr_name}
EOV
generated_attribute_methods.module_eval(method_body, __FILE__, line)
else
Expand Down

6 comments on commit 86acbf1

@mikezter
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This created a lot of confusion today, as we used underscore-prefixed methods in our model code ourselves. Also, i think changes like this should not be part of a minor version bump or a security release.

@raroni
Copy link

@raroni raroni commented on 86acbf1 Apr 12, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit causes our Rails app to run very slowly and to use about twice the amount of RAM. We tested the following code snippet with Rails at this and the previous commit. With this commit it ran around 15 times slower than with the previous commit.

c = User.first
1000000.times do
  c.name
end

We're running Ruby 1.8, so we thought we'd try the same benchmark with Ruby 1.9. With 1.9 there were no problems at all.

In order to fix the performance regression right here and now, we have downgraded to Rails 3.0.5.

My best guess as to why this is happening is that Object#send is much more expensive on 1.8 than 1.9.

@josevalim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was reverted though before 3.0.6. @tenderlove, c/d?

@raroni
Copy link

@raroni raroni commented on 86acbf1 Apr 12, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean it isn't a part of the final 3.0.6 release?

As far as I can see, it is: https://github.com/rails/rails/blob/v3.0.6/activerecord/lib/active_record/attribute_methods/read.rb
It is even mentioned in the release notes: http://weblog.rubyonrails.org/2011/4/6/rails-3-0-6-has-been-released

@josevalim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RasmusRn, you are right. So we were planning to revert it but we didn't. Maybe @jeremy or @tenderlove have something to add, let's wait.

@tenderlove
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit fixes a bug. The problem is outlined in the comments on 0823bbd

We need to find a way other than reverting this commit to fix the slowness. ❤️

Please sign in to comment.