rsl / stringex

Some [hopefully] useful extensions to Ruby’s String class. It is made up of three libraries: ActsAsUrl [permalink solution with better character translation], Unidecoder [Unicode to Ascii transliteration], and StringExtensions [miscellaneous helper methods for the String class].
MIT License
984 stars 158 forks source link

Freeze the "." character used in comparisons #181

Open Darep opened 8 years ago

Darep commented 8 years ago

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

rsl commented 8 years ago

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

Darep commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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