Consistently Frozen Constants

[avit]

Looking at Ransack::Constants, it looks like most strings are frozen for optimization but there are also arrays with unfrozen strings via %w(s t r).freeze. I suspect this is an oversight.

Is there any benefit if the arrays themselves are frozen too? (Personally I do have a case for modifying the arrays e.g. #123.)

[jonatack]

Hi @avit, good points. Things are evolving, but with Ruby 2.2 here is my current understanding:

• I believe you are correct that the strings inside the array could benefit from freezing, in hot spots.
• Freezing a string may now be faster than looking up a frozen string constant.
• Symbols are now GCed, and have usually been fast to look up, so they may be a useful option.
• String keys in hashes seem to have been optimized too, so no need anymore to freeze them and no speed advantage in using symbol hash keys instead.

So, it might make sense now in Ransack master and upcoming releases, to optimize for Ruby 2.2+ and stop using frozen constants, and replace them with frozen strings or symbols.

[avit]

I wasn't really considering symbols yet. When it comes to frozen strings vs. symbols I think you're right, they are essentially the same performance-wise and GC-wise now. Also, GC shouldn't be a concern since these "constants" are a fixed list of items: as long as we're not symbolizing user input like rails params used to do. The other considerations for String/Symbol are:

• "key".freeze is ugly. :key is not.
• Method interface is different. Are we using the objects for their content or just identifiers?
• Hash access could get confusing unless we depend on HashWithIndifferentAccess to bridge it (which uses string keys under the hood anyway).

Targeting Ruby 2.2 for optimization going forward makes sense but I think we should be careful if we start doing anything like to_sym or symbolize_keys for user input under ruby < 2.2, especially with things like ransack-ui. I think symbols might be a later consideration.

For now, I propose just changing the Ransack::Constants arrays as %(s t r).map(&:freeze) instead of freezing the arrays and not the strings.

[avit]

(If you want me to pull the declared constants e.g. EQ_ANY into regular frozen strings to skip constant lookup I can do that too.)

[jonatack]

Sounds good 👍

On Tuesday, April 7, 2015, Andrew Vit notifications@github.com wrote:

(If you want me to pull the declared constants e.g. EQ_ANY into regular
frozen strings to skip constant lookup I can do that too.)

—
Reply to this email directly or view it on GitHub
#530 (comment)
.