Permalink
Browse files

avoid creating an object in every call

This was a suggestion of @carlosantoniodasilva, thanks!
  • Loading branch information...
1 parent 00928bb commit 2d9c4017dd294e336e0245251f67bf490370305d @fxn fxn committed Jan 22, 2013
Showing with 2 additions and 1 deletion.
  1. +2 −1 actionpack/lib/action_controller/metal/strong_parameters.rb
@@ -401,14 +401,15 @@ def array_of_permitted_scalars_filter(params, key)
end
end
+ EMPTY_ARRAY = []
def hash_filter(params, filter)
filter = filter.with_indifferent_access
# Slicing filters out non-declared keys.
slice(*filter.keys).each do |key, value|
return unless value
- if filter[key] == []
+ if filter[key] == EMPTY_ARRAY
# Declaration { comment_ids: [] }.
array_of_permitted_scalars_filter(params, key)
else

14 comments on commit 2d9c401

❤️

Member

vipulnsward replied Jan 22, 2013

cool.

Owner

pixeltrix replied Jan 22, 2013

Ruby should really optimise these kind of things automatically 😢

Contributor

bogdan replied Jan 22, 2013

I can confirm what @pixeltrix has said. I even did a benchmark for this. Absolutely no difference.

Owner

fxn replied Jan 22, 2013

What @pixeltrix said is that Ruby should optimize this. He said it because it doesn't :).

So the patch is not going to give you a 10% performance boost, it is a humble patch that creates one single object rather than an object per call. No big deal, Ruby has hundreds of thousands of objects probably, but we do not need to create objects arbitrarily, specially if the fix is so easy.

Owner

fxn replied Jan 22, 2013

Oh, in case it is not clear, this patch targets the garbage collector, not speed.

Owner

pixeltrix replied Jan 22, 2013

@bogdan there are quite a few of these optimisations throughout the Rails codebase where things like Regexps have been moved to constants. As @fxn pointed out, I meant Ruby should optimise these by detecting that it's never assigned and only used for comparison.

Contributor

tilsammans replied Jan 24, 2013

Wouldn't filter[key].empty? be better still?

@tilsammans that passes for hashes as well, so it'd need an is_a? Array check.

Owner

fxn replied Jan 24, 2013

Responding to empty? is weaker, we must ensure it is an array.

Contributor

tilsammans replied Jan 24, 2013

I see!

Owner

pixeltrix replied Jan 24, 2013

@tilsammans no, it's explicitly testing for an empty array - "".empty? returns true as well

Owner

pixeltrix replied Jan 24, 2013

@tilsammans sorry for the Core Team smackdown - it happens that way sometimes 😄

Contributor

tilsammans replied Jan 24, 2013

Not at all -- I just hadn't considered that an empty hash isn't supposed to pass. No one was harmed under this particular pile-on :)

Please sign in to comment.