stringex
stringex copied to clipboard
Freeze the "." character used in comparisons
(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.
wondering should this benefit be extended anywhere else? looks good though.
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 :)
Hello all,
after digging a little more i found
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?
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
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