Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

AS::Callbacks: Refactor :per_key option #4493

Merged
merged 1 commit into from

3 participants

@bogdan

I suppose that :per_key option was originally designed to improve performance of (before/after/around)_filter in ActionController. Maybe someone who is in Rails core team since 2009 can explain why this option appeared.

But now when overall performance issue was fixed, we can rewrite it with 3 lines of code using other code we have.

Performance test: https://gist.github.com/1626009

19% performance improvement on set_callback
19% performance improvement on run_callbacks with :per_key option
15% performance improvement on skip_callback

Checked tests for AR, AM and AP to ensure everything is fine.

Rehearsal ----------------------------------------------------
New set_callback   0.050000   0.000000   0.050000 (  0.045225)
Old set_callback   0.050000   0.010000   0.060000 (  0.056748)
------------------------------------------- total: 0.110000sec

                       user     system      total        real
New set_callback   0.040000   0.000000   0.040000 (  0.043966)
Old set_callback   0.050000   0.000000   0.050000 (  0.053711)
************************************************************************************************************************
Rehearsal --------------------------------------------------------
New define_callbacks   0.020000   0.000000   0.020000 (  0.019756)
Old define_callbacks   0.020000   0.000000   0.020000 (  0.019723)
----------------------------------------------- total: 0.040000sec

                           user     system      total        real
New define_callbacks   0.010000   0.010000   0.020000 (  0.019719)
Old define_callbacks   0.020000   0.010000   0.030000 (  0.019841)
************************************************************************************************************************
Rehearsal -----------------------------------------------------
New run_callbacks   0.000000   0.000000   0.000000 (  0.004363)
Old run_callbacks   0.010000   0.000000   0.010000 (  0.005427)
-------------------------------------------- total: 0.010000sec

                        user     system      total        real
New run_callbacks   0.000000   0.000000   0.000000 (  0.004433)
Old run_callbacks   0.010000   0.000000   0.010000 (  0.005051)
************************************************************************************************************************
Rehearsal --------------------------------------------------------------
New run_callbacks with key   0.000000   0.000000   0.000000 (  0.000471)
Old run_callbacks with key   0.000000   0.000000   0.000000 (  0.001329)
----------------------------------------------------- total: 0.000000sec

                                 user     system      total        real
New run_callbacks with key   0.000000   0.000000   0.000000 (  0.000473)
Old run_callbacks with key   0.000000   0.000000   0.000000 (  0.000570)
************************************************************************************************************************
Rehearsal -----------------------------------------------------
New skip_callback   0.020000   0.010000   0.030000 (  0.029028)
Old skip_callback   0.030000   0.000000   0.030000 (  0.033500)
-------------------------------------------- total: 0.060000sec

                        user     system      total        real
New skip_callback   0.030000   0.000000   0.030000 (  0.028263)
Old skip_callback   0.030000   0.000000   0.030000 (  0.033339)

@josevalim
Owner

/cc @wycats

@tenderlove tenderlove commented on the diff
activesupport/lib/active_support/callbacks.rb
@@ -124,8 +118,9 @@ def normalize_options!(options)
options[:unless] = Array(options[:unless])
options[:per_key] ||= {}
- options[:per_key][:if] = Array(options[:per_key][:if])
- options[:per_key][:unless] = Array(options[:per_key][:unless])
+
+ options[:if] += Array(options[:per_key][:if])
+ options[:unless] += Array(options[:per_key][:unless])
@tenderlove Owner

I think you can use Array#concat in this case to avoid creating more arrays. Probably doesn't matter though. :-/

@bogdan
bogdan added a note

There are many tiny improvement that should be done.

But I just want to keep focus on problems that will give a significant performance boost and/or reduce complexity.
I will move to details as soon all main problems will be fixed,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove
Owner

This patch seems good to me.

@josevalim josevalim merged commit 422958f into rails:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 17, 2012
  1. @bogdan
This page is out of date. Refresh to see the latest.
Showing with 7 additions and 35 deletions.
  1. +7 −35 activesupport/lib/active_support/callbacks.rb
View
42 activesupport/lib/active_support/callbacks.rb
@@ -91,29 +91,23 @@ def halted_callback_hook(filter)
class Callback #:nodoc:#
@@_callback_sequence = 0
- attr_accessor :chain, :filter, :kind, :options, :per_key, :klass, :raw_filter
+ attr_accessor :chain, :filter, :kind, :options, :klass, :raw_filter
def initialize(chain, filter, kind, options, klass)
@chain, @kind, @klass = chain, kind, klass
normalize_options!(options)
- @per_key = options.delete(:per_key)
@raw_filter, @options = filter, options
@filter = _compile_filter(filter)
@compiled_options = _compile_options(options)
@callback_id = next_id
-
- _compile_per_key_options
end
def clone(chain, klass)
obj = super()
obj.chain = chain
obj.klass = klass
- obj.per_key = @per_key.dup
obj.options = @options.dup
- obj.per_key[:if] = @per_key[:if].dup
- obj.per_key[:unless] = @per_key[:unless].dup
obj.options[:if] = @options[:if].dup
obj.options[:unless] = @options[:unless].dup
obj
@@ -124,8 +118,9 @@ def normalize_options!(options)
options[:unless] = Array(options[:unless])
options[:per_key] ||= {}
- options[:per_key][:if] = Array(options[:per_key][:if])
- options[:per_key][:unless] = Array(options[:per_key][:unless])
+
+ options[:if] += Array(options[:per_key][:if])
+ options[:unless] += Array(options[:per_key][:unless])
@tenderlove Owner

I think you can use Array#concat in this case to avoid creating more arrays. Probably doesn't matter though. :-/

@bogdan
bogdan added a note

There are many tiny improvement that should be done.

But I just want to keep focus on problems that will give a significant performance boost and/or reduce complexity.
I will move to details as soon all main problems will be fixed,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
def name
@@ -147,22 +142,11 @@ def _update_filter(filter_options, new_options)
def recompile!(_options, _per_key)
_update_filter(self.options, _options)
- _update_filter(self.per_key, _per_key)
+ _update_filter(self.options, _per_key)
@callback_id = next_id
@filter = _compile_filter(@raw_filter)
@compiled_options = _compile_options(@options)
- _compile_per_key_options
- end
-
- def _compile_per_key_options
- key_options = _compile_options(@per_key)
-
- @klass.class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
- def _one_time_conditions_valid_#{@callback_id}?
- true if #{key_options}
- end
- RUBY_EVAL
end
# Wraps code with filter
@@ -200,11 +184,6 @@ def apply(code, key=nil, object=nil)
end
end
-
- def one_time_conditions_valid?(object)
- object.send("_one_time_conditions_valid_#{@callback_id}?")
- end
-
private
# Compile around filters with conditions into proxy methods
@@ -341,7 +320,7 @@ def compile(key=nil, object=nil)
method << "halted = false"
callbacks = yielding
- applicable_callbacks_for(key, object).reverse_each do |callback|
+ reverse_each do |callback|
callbacks = callback.apply(callbacks, key, object)
end
method << callbacks
@@ -369,13 +348,6 @@ def yielding
method.join("\n")
end
- # Selects callbacks that have valid <tt>:per_key</tt> condition
- def applicable_callbacks_for(key, object)
- return self unless key
- select do |callback|
- callback.one_time_conditions_valid?(object)
- end
- end
end
module ClassMethods
@@ -402,7 +374,7 @@ def __reset_runner(symbol)
end
def __callback_runner_name(key, kind)
- "_run__#{self.name.hash.abs}__#{kind}__#{key.hash.abs}__callbacks"
+ "_run__#{self.name.hash.abs}__#{kind}__callbacks"
end
# This is used internally to append, prepend and skip callbacks to the
Something went wrong with that request. Please try again.