From d8199003b863dc03bb7c3d6360e8faf897663c4f Mon Sep 17 00:00:00 2001 From: Markus Doits Date: Fri, 9 Sep 2016 14:48:03 +0200 Subject: [PATCH 1/4] Lower GC pressure in case of call with local variables By using the `missing_interpolation_argument_handler` and not always merging the globals with local interpolation values, it does not create a new hash for each call anymore. Lowers GC pressure noticable. --- lib/i18n/globals.rb | 30 +++++++++++++++++++++----- test/test_i18n_globals.rb | 45 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/lib/i18n/globals.rb b/lib/i18n/globals.rb index cd3e712..bc3dfe5 100644 --- a/lib/i18n/globals.rb +++ b/lib/i18n/globals.rb @@ -2,6 +2,8 @@ require 'i18n/globals/version' module I18n + FAKE_INTERPOLATION_HASH = { fake_: :_interpolation }.freeze + class Config class CachedGlobals < Hash def []=(key, val) @@ -79,15 +81,33 @@ def globals def globals=(new_globals) globals.clear.merge!(new_globals) # maybe there is something better than `clear` and `merge!` end + + # Overrides original I18n::Config#missing_interpolation_argument_handler + def missing_interpolation_argument_handler + @@missing_interpolation_argument_handler ||= lambda do |missing_key, provided_hash, string| + raise MissingInterpolationArgument.new(missing_key, provided_hash, string) + end + + @@missing_interpolation_argument_handler_with_globals ||= lambda do |missing_key, provided_hash, string| + # Since the key was not found in a interpolation variable, check + # whether it is a global. If it is, return it, so interpolation is + # successfull. + if globals.for_locale(locale).key?(missing_key) + globals.for_locale(locale)[missing_key] + else + @@missing_interpolation_argument_handler.call(missing_key, provided_hash, string) + end + end + end end class << self def translate(*args) - if args.last.is_a?(Hash) - args[-1] = config.globals.for_locale(locale).merge(args.last) - else - args << config.globals.for_locale(locale) - end + # If last argument is a hash, interpolation will be run. If not, it will + # not even attempt to interpolate something and our custom + # `missing_interpolation_argument_handler` will not be run at all. Thats + # why we pass in a fake hash so that it always runs interpolation. + args << FAKE_INTERPOLATION_HASH unless args.last.is_a?(Hash) super(*args) end diff --git a/test/test_i18n_globals.rb b/test/test_i18n_globals.rb index 33a288e..4751b22 100644 --- a/test/test_i18n_globals.rb +++ b/test/test_i18n_globals.rb @@ -168,4 +168,49 @@ def test_that_cache_is_cleared_after_merging_a_new_global assert_equal 'Hi there, Dobby!', I18n.translate('greeting') end + + def test_it_still_fails_on_missing_interpolation + assert_raises(I18n::MissingInterpolationArgument) { I18n.translate('greeting') } + end + + def test_it_allows_to_set_a_custom_missing_interpolation_argument_handler + I18n.config.missing_interpolation_argument_handler = proc { raise 'works!' } + + assert_raises('works!') { I18n.translate('greeting') } + + I18n.config.missing_interpolation_argument_handler = nil + end + + def test_it_translates_globals_with_custom_missing_interpolation_argument_handler + I18n.config.missing_interpolation_argument_handler = proc { raise 'works!' } + + I18n.config.globals = { + name: 'Greg' + } + + assert_equal I18n.translate('greeting'), 'Hi there, Greg!' + + I18n.config.missing_interpolation_argument_handler = nil + end + + def test_it_does_not_polute_the_object_space_with_hashes + I18n.config.globals = { + name: 'Greg' + } + + values = { name: 'Dobby' } + times = 10_000 + + GC.disable + count_before = ObjectSpace.each_object(Hash).count + times.times { I18n.translate('greeting', values) } + count_after = ObjectSpace.each_object(Hash).count + GC.enable + + # It looks like I18n.translate by default allocates 2 new hashes + # per call. So substract it from the count. + expected_count = count_after - times * 2 + + assert_operator count_before, :==, expected_count + end end From 54fac08a897d1fc32e25989d7d3ec0019a78b5ad Mon Sep 17 00:00:00 2001 From: Markus Doits Date: Fri, 9 Sep 2016 15:36:46 +0200 Subject: [PATCH 2/4] Use #prepend instead of overriding method Because: No copy and pasted code from I18n --- lib/i18n/globals.rb | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/i18n/globals.rb b/lib/i18n/globals.rb index bc3dfe5..bcb12a7 100644 --- a/lib/i18n/globals.rb +++ b/lib/i18n/globals.rb @@ -82,23 +82,23 @@ def globals=(new_globals) globals.clear.merge!(new_globals) # maybe there is something better than `clear` and `merge!` end - # Overrides original I18n::Config#missing_interpolation_argument_handler - def missing_interpolation_argument_handler - @@missing_interpolation_argument_handler ||= lambda do |missing_key, provided_hash, string| - raise MissingInterpolationArgument.new(missing_key, provided_hash, string) - end - - @@missing_interpolation_argument_handler_with_globals ||= lambda do |missing_key, provided_hash, string| - # Since the key was not found in a interpolation variable, check - # whether it is a global. If it is, return it, so interpolation is - # successfull. - if globals.for_locale(locale).key?(missing_key) - globals.for_locale(locale)[missing_key] - else - @@missing_interpolation_argument_handler.call(missing_key, provided_hash, string) + prepend( + Module.new do + def missing_interpolation_argument_handler + @@missing_interpolation_argument_handler_with_globals ||= + lambda do |missing_key, provided_hash, string| + # Since the key was not found in a interpolation variable, check + # whether it is a global. If it is, return it, so interpolation is + # successfull. + if globals.for_locale(locale).key?(missing_key) + globals.for_locale(locale)[missing_key] + else + super.call(missing_key, provided_hash, string) + end + end end end - end + ) end class << self From a8b8472570057d4016a314785702b3c200c0f195 Mon Sep 17 00:00:00 2001 From: Markus Doits Date: Fri, 9 Sep 2016 15:43:51 +0200 Subject: [PATCH 3/4] drop support for ruby < 2 ? --- .travis.yml | 9 +++++++++ i18n-globals.gemspec | 2 ++ 2 files changed, 11 insertions(+) diff --git a/.travis.yml b/.travis.yml index f819a51..8c09207 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1 +1,10 @@ language: ruby +rvm: + - 2.0 + - 2.1 + - 2.2.5 + - 2.3.1 + - ruby-head +matrix: + allow_failures: + - rvm: ruby-head diff --git a/i18n-globals.gemspec b/i18n-globals.gemspec index 01f1eb9..34e4696 100644 --- a/i18n-globals.gemspec +++ b/i18n-globals.gemspec @@ -18,6 +18,8 @@ Gem::Specification.new do |spec| spec.test_files = spec.files.grep(%r{^(test|spec|features)/}) spec.require_paths = ['lib'] + spec.required_ruby_version = '>= 2.0' + spec.add_development_dependency 'bundler', '~> 1.6' spec.add_development_dependency 'rake' spec.add_development_dependency 'minitest' From 838b99355c1a730226cbba16d238f97815100de9 Mon Sep 17 00:00:00 2001 From: Markus Doits Date: Fri, 9 Sep 2016 16:29:52 +0200 Subject: [PATCH 4/4] make test a little bit clearer --- test/test_i18n_globals.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_i18n_globals.rb b/test/test_i18n_globals.rb index 4751b22..85778ad 100644 --- a/test/test_i18n_globals.rb +++ b/test/test_i18n_globals.rb @@ -209,8 +209,8 @@ def test_it_does_not_polute_the_object_space_with_hashes # It looks like I18n.translate by default allocates 2 new hashes # per call. So substract it from the count. - expected_count = count_after - times * 2 + created_due_to_globals = count_after - count_before - times * 2 - assert_operator count_before, :==, expected_count + assert_equal created_due_to_globals, 0 end end