Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AS::Callbacks: Refactor :per_key option #4493

Merged
merged 1 commit into from Jan 17, 2012
Merged

AS::Callbacks: Refactor :per_key option #4493

merged 1 commit into from Jan 17, 2012

Conversation

bogdan
Copy link
Contributor

@bogdan bogdan commented Jan 17, 2012

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
Copy link
Contributor

/cc @wycats

options[:per_key][:unless] = Array(options[:per_key][:unless])

options[:if] += Array(options[:per_key][:if])
options[:unless] += Array(options[:per_key][:unless])
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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,

@tenderlove
Copy link
Member

This patch seems good to me.

josevalim added a commit that referenced this pull request Jan 17, 2012
AS::Callbacks: Refactor :per_key option
@josevalim josevalim merged commit 422958f into rails:master Jan 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants