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

Deprecate passing string to define callback. #22598

Merged
merged 1 commit into from Dec 16, 2015

Conversation

yui-knk
Copy link
Contributor

@yui-knk yui-knk commented Dec 15, 2015

No description provided.

@rails-bot
Copy link

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

@yui-knk
Copy link
Contributor Author

yui-knk commented Dec 15, 2015

ref #22460

@claudiob
Copy link
Member

👍 @yui-knk 😄

As part of this commit, I think you should also remove this documentation.

@@ -48,7 +48,8 @@ def _normalize_callback_options(options)

def _normalize_callback_option(options, from, to) # :nodoc:
if from = options[from]
from = Array(from).map {|o| "action_name == '#{o}'"}.join(" || ")
_from = Array(from).map(&:to_s)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should create a Set so the performance of include? is better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, looks like creating Sets is too slow. Here's a benchmark. With an Array of 10 elements, creating a Set and then calling #include? is 5x slower than creating an Array and calling #include?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, creating sets is slow but we are creating only once and we are calling include on every request so it may be worth using Set here.

Copy link
Member

Choose a reason for hiding this comment

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

Per the size of the Array it will not matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nateberkopec Thanks for your help

$ ruby creating_set.rb
Calculating -------------------------------------
         set include    45.093k i/100ms
       array include    15.519k i/100ms
-------------------------------------------------
         set include    713.458k (± 5.0%) i/s -      3.562M
       array include    182.015k (± 4.3%) i/s -    915.621k

Comparison:
         set include:   713457.8 i/s
       array include:   182014.8 i/s - 3.92x slower

@yui-knk
Copy link
Contributor Author

yui-knk commented Dec 16, 2015

@rafaelfranca I updatd 😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants