Permalink
Browse files

AS::Callbacks remove useless code, improve performance

  • Loading branch information...
bogdan committed May 16, 2012
1 parent 4edb497 commit 30b31f51af6f7094c4a27b086755fc66c368d6fa
Showing with 3 additions and 16 deletions.
  1. +3 −16 activesupport/lib/active_support/callbacks.rb
@@ -328,26 +328,17 @@ module ClassMethods
# if it was not yet defined.
# This generated method plays caching role.
def __define_callbacks(kind, object) #:nodoc:
- name = __callback_runner_name(kind)
+ chain = object.send("_#{kind}_callbacks")
+ name = "_run_callbacks_#{chain.object_id}"
unless object.respond_to?(name, true)
- str = object.send("_#{kind}_callbacks").compile
class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
- def #{name}() #{str} end
+ def #{name}() #{chain.compile} end
protected :#{name}
RUBY_EVAL
end
name
end
- def __reset_runner(symbol)
- name = __callback_runner_name(symbol)
- undef_method(name) if method_defined?(name)
- end
-
- def __callback_runner_name(kind)
- "_run__#{self.name.hash.abs}__#{kind}__callbacks"
- end
-
# This is used internally to append, prepend and skip callbacks to the
# CallbackChain.
#
@@ -359,7 +350,6 @@ def __update_callbacks(name, filters = [], block = nil) #:nodoc:
([self] + ActiveSupport::DescendantsTracker.descendants(self)).reverse.each do |target|
chain = target.send("_#{name}_callbacks")
yield target, chain.dup, type, filters, options
- target.__reset_runner(name)
end
end
@@ -447,12 +437,9 @@ def reset_callbacks(symbol)
chain = target.send("_#{symbol}_callbacks").dup
callbacks.each { |c| chain.delete(c) }
target.send("_#{symbol}_callbacks=", chain)
- target.__reset_runner(symbol)
end
self.send("_#{symbol}_callbacks=", callbacks.dup.clear)
-
- __reset_runner(symbol)
end
# Define sets of events in the object lifecycle that support callbacks.

13 comments on commit 30b31f5

Contributor

bogdan replied May 17, 2012

Never though object_id could be negative. We should do abs in there.

Member

drogus replied May 17, 2012

@bogdan wouldn't it be better to tr '-' into '_' ? I know that's highly unlikely, but abs could give the same object_id as other object. I don't know what are the chances, though, maybe that's something that we can ignore.

Owner

spastorino replied May 17, 2012

Yep when I first saw this I thought the same thing @drogus is mentioning. @bogdan can you provide a PR to fix this? thanks :)

Contributor

bogdan replied May 18, 2012

Member

drogus replied May 18, 2012

I doubt that it will change performance in any meaningful way:

>> Benchmark.measure { 1_000_000.times { "_run__#{no}___callbacks".tr("-", "_") } }.to_s
=> "  1.310000   0.000000   1.310000 (  1.324470)\n"
>> Benchmark.measure { 1_000_000.times { "_run__#{no.abs}___callbacks" } }.to_s
=> "  0.690000   0.000000   0.690000 (  0.700540)\n"

That said, if that's the way this code worked, I guess it can stay with abs.

Contributor

josevalim replied May 18, 2012

Yeah, performance should not matter in this case because this is run just once per class in the app lifecycle. We should just have a patch and fix the build asap.

Contributor

bogdan replied May 18, 2012

@josevalim Unfortunatelly this is not true. Runner method name is build every time #run_callbacks is called. Performance matter here.

Contributor

josevalim replied May 18, 2012

Contributor

bogdan replied May 18, 2012

Good idea. But if cache will be done in instance variable - this will require to flush the cache in initialize_dup or thing like this so that dup object won't share runner method name

Owner

rafaelfranca replied Jun 1, 2012

@bogdan this commit make the Active Model suite to brake sometimes. Reverting this commit all the tests pass.

I used this script to run the suite many times.

With this commit: 50 times, 3 failures
Without this commit: 100 times, 0 failures

Could you investigate?

Contributor

bogdan replied Jun 1, 2012

Sure,

Can you share some additional information with me:

  • a failure message
  • what are your thoughts on the reason
  • how did you detect this issue?

Thanks for detecting this. Can't even imagine how hard it could be to detect.

Owner

rafaelfranca replied Jun 1, 2012

These are some examples of broken builds:

  1. http://travis-ci.org/#!/rails/rails/jobs/1498992
  2. http://travis-ci.org/#!/rails/rails/jobs/1496948
  3. http://travis-ci.org/#!/rails/rails/jobs/1489985

These errors started on the same day that we merged this commit, but we don't started to investigate, so I tried to revert this commit today because I thought it was the only reason to make the validations tests break, because we don't changed anything related with validations.

I think that we are getting callbacks name collision, and one of reason to this thought is this failure:

[/home/vagrant/builds/rails/rails/activemodel/lib/active_model/errors.rb:360]:
unexpected invocation: #<ActiveModel::Errors:0x9cbc5c4>.generate_message(:title, :accepted, {})
unsatisfied expectations:
- expected exactly once, not yet invoked: #<ActiveModel::Errors:0x9cbc5c4>.generate_message(:title, :less_than, {:value => 1, :count => 0})
Please sign in to comment.