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

Freeze the "." character used in comparisons #181

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Darep
Copy link

@Darep Darep commented Feb 10, 2016

(Disclaimer: this change is written by @Fred-JulieDesk, I take no credit or responsibility, I'm just the pull request creator :) )

This change freezes the . character that is used in comparison of the translation key. This way, we don't allocate a new string object every time the comparison is made and save a bit of memory.

See #180 for more.

@@ -11,6 +11,9 @@ module Localization
include DefaultConversions

class << self

DOT = ".".freeze

Copy link
Collaborator

Choose a reason for hiding this comment

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

Trailing whitespace detected.

@Darep Darep mentioned this pull request Feb 10, 2016
@rsl
Copy link
Owner

rsl commented Feb 10, 2016

wondering should this benefit be extended anywhere else? looks good though.

@Darep
Copy link
Author

Darep commented Feb 10, 2016

I guess @Fred-JulieDesk might be able to chime in on that :) Did the memory allocation report reveal any other strings that allocate considerable amount of memory? Personally I think it's better to leave the other static strings be if there's no evidence that they're harmful/eating memory :)

@Fred-JulieDesk
Copy link

Hello all,

after digging a little more i found
capture d ecran 2016-02-10 a 15 29 10

In lib/stringex/localization/default_conversions I guess you could freeze all these strings as they are not supposed to change during code execution (I think).

Also the litteral array %w{characters currencies html_entities transliterations vulgar_fractions}, instead maybe you could use a hash with the upcased string frozen.

So, this:

%w{characters currencies html_entities transliterations vulgar_fractions}.each do |conversion_type|
          define_method conversion_type do
            const_get conversion_type.upcase
          end
        end

would become something like this:

CONVERSION_TYPES = {
        characters: 'CHARACTERS'.freeze,
        currencies: 'CURRENCIES'.freeze,
        html_entities: 'HTML_ENTITIES'.freeze,
        transliterations: 'TRANSLITERATIONS'.freeze,
        vulgar_fractions: 'VULGAR_FRACTIONS'.freeze,
      }

%w{characters currencies html_entities transliterations vulgar_fractions}.each do |conversion_type|
          define_method conversion_type do
            const_get CONVERSION_TYPES[conversion_type]
          end
        end

What do you think?

@Darep
Copy link
Author

Darep commented Feb 13, 2016

In lib/stringex/localization/default_conversions I guess you could freeze all these strings as they are not supposed to change during code execution (I think).

Nice find! Though I think this might actually make loading this file in a bit slower (a very very tiny fraction slower) because the strings are already constants so they are only initialized once (in my understanding). So adding .freeze to these would add an extra operation per string which would make that bit "slower" :) I'd leave them as is. The amount of memory "TRANSLITERATIONS" is using looks to me to be around the same as the frozen . character

@Fred-JulieDesk
Copy link

For the constantes part, it is a good practice to freeze them to forbid any ulterior modification as Ruby constantes are mutable. About the "TRANSLITERATIONS" part, the string is instantiated each time the method to get the conversion type is called (moreover it is true for every other conversion type). Also, each times the method is called, the string is upcased which result in a serious overhead and useless string transformations when it is called a lot.
By freezing the uppercased string directly we ensure to reduce this overhead and useless string transformations. It may not have a huge impact on modest applications but for larger applications making extensive usage of this gem i really think it would be a nice improvment with in my understanding no drawbacks

const_get conversion_type.upcase
# %w{characters currencies html_entities transliterations vulgar_fractions}

[['characters'.freeze, 'CHARACTERS'.freeze], ['currencies'.freeze, 'CURRENCIES'.freeze], ['html_entities'.freeze, 'HTML_ENTITIES'.freeze], ['transliterations'.freeze, 'TRANSLITERATIONS'.freeze], ['vulgar_fractions'.freeze, 'VULGAR_FRACTIONS'.freeze]].each do |conversion_type|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is too long. [284/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

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.

4 participants