lookup method not reusing @translations about i18n-active_record HOT 12 CLOSED

Yes, that's correct, we hit DB every time the i18n.t is called
There is no caching except the rails SQL caching

There is a PR that adds cache_translations config option. With this option set to true the translations table will be cached on the first I18n.t call.

from i18n-active_record.

Hi @dvkch,

I think the problem here is that the protected lookup calls Translation.lookup under the hood (https://github.com/svenfuchs/i18n-active_record/blob/master/lib/i18n/backend/active_record/translation.rb#L62-L73)
So if we want to add any performance optimization we would need to somehow emulate the Translation.lookup method behaviour

If you have an idea how to do that, feel free to open a PR

from i18n-active_record.

I’ll draw up a pour when I’ll be at a computer.
In the mean time here was my thinking.

I reimplemented the same behavior as this library, but replacing the locale and value field by a jsonb value field, storing a hash of locale to value.

I did not have to implement the Translation.lookup method and instead the backend lookup basically turned into @translations.dig(*keys). I added a callback after a Translation update transaction commit to clear the cached @translations. This turns out nicely performance wise since loading translations is a single SQL request and they shouldn’t be updated often, and the code is more readable but I donnot have any benchmark to show.

from i18n-active_record.

Correct me if I am wrong, so you are basically keeping one translation row per locale and all translations for the given locale are stored in the jsonb field?

from i18n-active_record.

The opposite. I have a row per key and the value jsonb field contains values for each locale. This allowed me to add a unique index to the key column and easily create an ActiveAdmin page for my Translation model, allowing me to show/edit all localizations for a key at once. This is why I cannot easily send you a PR since the internals are a bit different. But here is the backend class, it is still very similar :

require 'i18n/backend/base'

# heavily inspired from https://github.com/svenfuchs/i18n-active_record/, but adapted to work nicely with Mobility gem
module I18n
  module Backend
    class ActiveRecord
      module Implementation
        include Base, Flatten

        def available_locales
          ::Translation.available_locales
        end

        def store_translations(locale, data, options = {})
          escape = options.fetch(:escape, true)
          create_only = options.fetch(:create_only, false)

          Translation.transaction do
            flatten_translations(locale, data, escape, false).each do |key, value|
              # cleanup conflicts, e.g.: can't have "common.actions" defined if "common.actions.new" is being written...
              conflicting_translations = ::Translation.where(key: conflicting_keys(key))
              conflicting_translations.destroy_all

              # ... and vice versa
              conflicting_translations = ::Translation.where('key LIKE ?', key.to_s + '.%')
              conflicting_translations.destroy_all

              # create new translation
              translation = ::Translation.find_or_initialize_by(key: key)
              next if create_only && translation.value(locale: locale, fallback: false).present?

              translation.update("value_#{locale}" => value)
            end
          end

          reload!
        end

        def reload!
          @translations = nil
          self
        end

        def initialized?
          !@translations.nil?
        end

        def init_translations
          if Translation.table_exists?
            @translations = ::Translation.to_hash
          else
            @translations = {}
          end
        end

        def translations(do_init: false)
          init_translations if do_init || !initialized?
          @translations ||= {}
        end

        protected

        def lookup(locale, key, scope = [], options = EMPTY_HASH)
          # flatten the key, e.g.: key="actions.new", scope=["common"] => common.actions.new
          key = normalize_flat_keys(locale, key, scope, options[:separator])

          # remove leading and trailing dots
          key = key.delete_prefix('.').delete_suffix('.')

          # fetch results
          keys = [locale.to_sym] + key.split(I18n::Backend::Flatten::FLATTEN_SEPARATOR).map(&:to_sym)
          translations.dig(*keys)
        end

        # For a key :'foo.bar.baz' return ['foo', 'foo.bar', 'foo.bar.baz']
        def expand_keys(key)
          key.to_s.split(FLATTEN_SEPARATOR).inject([]) do |keys, key|
            keys << [keys.last, key].compact.join(FLATTEN_SEPARATOR)
          end
        end

        def conflicting_keys(key)
          expand_keys(key) - [key.to_s]
        end
      end

      include Implementation
    end
  end
end

As you can see the lookup method can easily use the translations method itself, and find the corresponding value or hash in it, instead of relying on Translation to do it.

The Translation model looks like this (I removed some other features that are not related to this issue):

class Translation < ApplicationRecord
  translates :value # Mobility gem

  after_commit :reload_translations

  scope :locale, ->(locale) { Mobility.with_locale(locale) { i18n.where.not(value: nil) } }

  def self.available_locales
    Translation.select('DISTINCT jsonb_object_keys(value_i18n) AS locale').to_a.map(&:locale)
  end

  def self.to_hash
    all.each.with_object({}) do |t, hash|
      locales = t.value_i18n.keys
      locales.each do |locale|
        keys = [locale.to_sym] + t.key.split(I18n::Backend::Flatten::FLATTEN_SEPARATOR).map(&:to_sym)
        keys.each.with_index.inject(hash) do |iterator, (key, index)|
          if index == keys.size - 1
            iterator[key] = t.value(locale: locale, fallback: false)
          else
            iterator[key] ||= {}
          end
          iterator[key]
        end
      end
    end
  end

  validates :key, presence: true, uniqueness: true

  protected

  def reload_translations
    backend = I18n.backend
    backend = backend.backends.find { |b| b.is_a?(I18n::Backend::ActiveRecord) } if backend.is_a?(I18n::Backend::Chain)
    backend.reload!
  end
end

I think this is a nice readability improvement and the developer can now assume the primary source of information is the translations method of the backend. In this case there is no longer need for complicated queries or memoization, the trade off being that everything translation is kept in memory.

from i18n-active_record.

Hm, I see what you mean. I haven't thought about all edge case but it might a good addition that can be used to solve performance-related issues.
There is a way to configure the ActiveRecord backend via the configure call(e.g. I18n::Backend::ActiveRecord.configure do |config|), we could add an extra configuration option and change the #lookup method to support both Translation and @translations lookup(Translation being default)

from i18n-active_record.

Absolutely. There may be edge cases, I haven't been able to fall into some for now, and your test suite works with my fork, adapted for the parts that have been changed on my end of course.

from i18n-active_record.

Great, hiding the feature behind a configuration option will allow us to minimize regressions.
So you are more than welcome to bring the feature from your fork to this repo!

from i18n-active_record.

I'll get to it as soon as I have the time :) i'm not very well versed in gem editing and packaging, plus my use case is a bit different so I'd rather not break too much things 😬

from i18n-active_record.

Sure! If you need any help just let me know

from i18n-active_record.

I've been keeping up with this as I noticed our application doing lots of queries to the db for translations. So I understand it correctly, are translations looked up in the db every time a call to I18n.t occurs, or is there currently some caching that occurs so that the db request only happens once per I18n.t('something')? If there is a cache, when would that cache be busted so that a new translation from the db could be loaded?

from i18n-active_record.

This has been addressed here #122

from i18n-active_record.