Permalink
Browse files

DRY class_attribute code

  • Loading branch information...
1 parent 1f870a2 commit d59208d7032e2be855a89ad8d4685cc08dd7cdb3 @alexandrz alexandrz committed Jul 29, 2012
Showing with 3 additions and 6 deletions.
  1. +3 −6 activesupport/lib/active_support/core_ext/class/attribute.rb
@@ -79,14 +79,12 @@ def self.#{name}?() !!#{name} end
def self.#{name}=(val)
singleton_class.class_eval do
- remove_possible_method(:#{name})
- define_method(:#{name}) { val }
+ redefine_method(:#{name}) { val }
end
if singleton_class?
class_eval do
- remove_possible_method(:#{name})
- def #{name}
+ redefine_method(:#{name}) do
defined?(@#{name}) ? @#{name} : singleton_class.#{name}
end
end
@@ -95,8 +93,7 @@ def #{name}
end
if instance_reader
- remove_possible_method :#{name}
- def #{name}
+ redefine_method(:#{name}) do
defined?(@#{name}) ? @#{name} : self.class.#{name}
end

12 comments on commit d59208d

Contributor

thedarkone replied Jul 29, 2012

While I agree with the first change, the last 2 changes will result in performance regressions (methods created via the defined_method calls end up being noticeably slower).

The last 2 changes should be reverted.

Owner

rafaelfranca replied Jul 29, 2012

Yes, I agree with @thedarkone.

@thedarkone would be great if you open a pull request showing the benchmarks and adding a comment why this is not using define_method. So we'll not have more pull requests changing these lines.

Just to clarify, I do know that these lines are slower than the method definition as before, but they look clearer, and there are some places where code has been left the way it is because of the same thought. I'm more than happy to revert it if everyone considers class_attribute to be a pain point of slowness, where we should focus on speed, otherwise I think we're probably fine.

Owner

pixeltrix replied Jul 30, 2012

There are a few class_attribute definitions in Active Record that may have an effect on the speed of object creation, so probably worth someone doing the performance numbers on that.

Contributor

josevalim replied Jul 30, 2012

@pixeltrix got it right on spot. It would be fine to go with redefine_method, except that class_attribute is a basic building block. So in this case, I would prefer to ask then shot (i.e. benchmark then change) rather than shot then ask.

Contributor

josevalim replied Jul 30, 2012

Reverted on 8601f73

Not sure it'd affect object creation. Anyway, I agree that it'd be better to have some perf numbers. Thanks.

Owner

pixeltrix replied Jul 30, 2012

The serialized_attributes hash is stored in a class attribute and is accessed whenever an AR class is initialized. I'm not saying that performance is affected significantly but if you're running a import script that's creating hundreds of thousands of AR objects even a small difference may be a significant time.

@pixeltrix Ok, thanks for clarifying. I'll play a little bit with this and AR when I find some time here.

Owner

tenderlove replied Mar 4, 2013

Do we actually have any benchmarks on the impact of changing these? I feel like we could fix AR such that we don't have a bottleneck on class method definitions.

The reason I ask this is that using class_eval maybe result in possibly faster methods, it definitely has an impact on memory usage and boot speed. If I apply this patch, I see a 1 megabyte drop in instruction sequence memory usage on my small app:

$ ruby script/rails runner -e production 'require "objspace"; GC.start; p ObjectSpace.memsize_of_all(RubyVM::InstructionSequence)'
26335368
$ ruby script/rails runner -e production 'require "objspace"; GC.start; p ObjectSpace.memsize_of_all(RubyVM::InstructionSequence)'
25285536

I suspect speed / memory benefits will be even better on large apps. From my measurements on MRI, the overhead isn't that large (though I think the profile is definitely different on JRuby). Can we at least have some measurements? Comments like this scare me because it encourages cargo culting of some particular technique without measurements. If these methods are indeed a runtime bottleneck, please demonstrate it. I'd like to reduce our boot speed / memory consumption, so I'm very interested in removing class_eval.

Contributor

thedarkone replied Mar 5, 2013

@headius @dbussink what are your thoughts on class_eval vs. define_method for hot methods.

Contributor

dbussink replied Mar 5, 2013

Rubinius at this point can't inline methods create with define_method. So having those methods in the hot path has detrimental performance effects, especially of these are small methods that otherwise would be inlined in surrounding code. The problem is that the actual effect of this very much depends on the context you're executing this in.

Compare for example these two benchmarks. The first one is the original from @tenderlove, the second slightly adapted for cases where method inlining has an effect. The biggest problem is that with such small methods, the overhead of the benchmark framework is significant so results can seem small but might be bigger in reality.

Original: https://gist.github.com/dbussink/91ebcb535f73ef2de412
Adapted: https://gist.github.com/dbussink/700a1d13d6ca8cf44469

I'm not saying that these numbers are what you should definitely go on and that define_method can't be optimized better. It's just that you have to be very careful about the effects of this because they have effect outside of just running that single method in a loop in runtimes other than MRI.

Please sign in to comment.