stringex icon indicating copy to clipboard operation
stringex copied to clipboard

Freeze the "." character used in comparisons

Open Darep opened this issue 9 years ago • 5 comments

(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.

Darep avatar Feb 10 '16 11:02 Darep

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

rsl avatar Feb 10 '16 12:02 rsl

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 :)

Darep avatar Feb 10 '16 13:02 Darep

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?

Fred-JulieDesk avatar Feb 10 '16 14:02 Fred-JulieDesk

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

Darep avatar Feb 13 '16 01:02 Darep

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

Fred-JulieDesk avatar Feb 15 '16 13:02 Fred-JulieDesk