From 58151ce461e559b7ba8a05a2fa9c9628f19c771b Mon Sep 17 00:00:00 2001 From: wangjohn Date: Thu, 2 May 2013 16:16:18 -0400 Subject: [PATCH] Improving the performance of callbacks by keeping a hash of the callbacks in the CallbackChain, so you don't have to iterate over all callbacks when checking for duplicates. Benchmark results when the tests in activerecord/test/cases/associations_test.rb were run with and without the change: == On master (when scanning all of the callbacks): --------------------------------------------------------- % cumulative self self total time seconds seconds calls ms/call ms/call name --------------------------------------------------------- 1.85 9.26 0.82 18412 0.04 0.05 ActiveSupport::Callbacks::Callback#matches? 1.22 12.32 0.54 18412 0.03 0.08 ActiveSupport::Callbacks::Callback#duplicates? 0.93 14.61 0.41 19600 0.02 0.21 ActiveSupport::Callbacks::CallbackChain#remove_duplicates Finished tests in 1.217065s, 30.4010 tests/s, 53.4072 assertions/s. --------------------------------------------------------- == On my branch (when using a hash to check callback duplication): --------------------------------------------------------- % cumulative self self total time seconds seconds calls ms/call ms/call name --------------------------------------------------------- 0.15 29.63 0.06 1188 0.05 0.72 ActiveSupport::Callbacks::CallbackChain#handle_duplicates 0.00 40.50 0.00 84 0.00 0.12 ActiveSupport::Callbacks::Callback#matches? 0.00 40.50 0.00 84 0.00 0.12 ActiveSupport::Callbacks::Callback#duplicates? 0.00 40.50 0.00 91 0.00 0.22 ActiveSupport::Callbacks::CallbackChain#scan_and_remove_duplicates Finished tests in 1.117757s, 33.1020 tests/s, 58.1522 assertions/s. --------------------------------------------------------- --- activesupport/lib/active_support/callbacks.rb | 44 ++++++++++++++----- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb index 893c2500d7..f0389d0a1c 100644 --- a/activesupport/lib/active_support/callbacks.rb +++ b/activesupport/lib/active_support/callbacks.rb @@ -132,6 +132,10 @@ module ActiveSupport @@_callback_sequence += 1 end + def object_filter? + @_is_object_filter + end + def matches?(_kind, _filter) if @_is_object_filter _filter_matches = @filter.to_s.start_with?(_method_name_for_object_filter(_kind, _filter, false)) @@ -337,6 +341,7 @@ module ActiveSupport :terminator => "false", :scope => [ :kind ] }.merge!(config) + @callbacks_hash = Hash.new { |h, k| h[k] = [] } end def compile @@ -361,20 +366,37 @@ module ActiveSupport private - def append_one(callback) - remove_duplicates(callback) - push(callback) - end + def append_one(callback) + handle_duplicates(callback) + push(callback) + end - def prepend_one(callback) - remove_duplicates(callback) - unshift(callback) - end + def prepend_one(callback) + handle_duplicates(callback) + unshift(callback) + end - def remove_duplicates(callback) - delete_if { |c| callback.duplicates?(c) } - end + # We check to see if this callback already exists. If it does (i.e. if + # +callback.duplicates?(c)+ for any callback +c+ in the list of + # callbacks), then we delete the previously defined callback. + # + # We make use of the rep-invariant that only one callback exists which + # might match the new callback. The +@callbacks_hash+ is keyed on the + # +kind+ and +filter+ of the callback, the attributes used to check if + # two callbacks match. + def handle_duplicates(callback) + if callback.object_filter? + scan_and_remove_duplicates(callback) + else + hash_key = [callback.kind, callback.filter] + delete @callbacks_hash[hash_key] if @callbacks_hash[hash_key] + @callbacks_hash[hash_key] = callback + end + end + def scan_and_remove_duplicates(callback) + delete_if { |c| callback.duplicates?(c) } + end end module ClassMethods