Improved action controller

[tagliala]

Based on how I18n.with_locale is implemented (https://github.com/svenfuchs/i18n/blob/master/lib/i18n.rb#L252) I tried to improve the action controller.

I aimed to avoid unnecessary function calls to I18n.with_locale and self.with_host_locale, assignments and nested block call. Moreover, the new approach should be faster but I'm very new to profiling so maybe you are interested in writing proper performance tests

Beside gem's own tests, I launched some of my applications specs:

Using route_translator 3.2.4 from source at ~/dev/route_translator

Finished in 2 minutes 36.1 seconds (files took 11.93 seconds to load)
317 examples, 0 failures

Using route_translator 3.2.4 from source at ~/dev/route_translator

Finished in 4 minutes 10.2 seconds (files took 11.38 seconds to load)
409 examples, 0 failures

Using route_translator 3.2.4 from source at ~/dev/route_translator

Finished in 1 minute 11.67 seconds (files took 11.85 seconds to load)
188 examples, 0 failures, 1 pending

[tagliala]

The diff sucks, here it is the whole method:

    def set_locale_from_url
      tmp_default_locale = RouteTranslator::Host.locale_from_host(request.host)
      if tmp_default_locale
        current_default_locale = I18n.default_locale
        I18n.default_locale    = tmp_default_locale
      end

      tmp_locale = params[RouteTranslator.locale_param_key] || tmp_default_locale
      if tmp_locale
        current_locale = I18n.locale
        I18n.locale    = tmp_locale
      end

      yield

    ensure
      I18n.default_locale = current_default_locale if tmp_default_locale
      I18n.locale = current_locale if tmp_locale
    end

[tagliala]

Of course we can do something like this:

    def set_locale_from_url
      I18n.with_locale(params[RouteTranslator.locale_param_key] || RouteTranslator::Host.locale_from_host(request.host)) do
        yield
      end
    end

but I suppose that also setting the default locale will have effects on fallbacks with gems like globalize, but there isn't a proper test

[enriclluelles]

Thanks man, appreciate the contribution, would you like commit bit in the project? I haven't had time for it lately even though people keep submitting issues now and then.

I'll check this deeply lately and probably merge it

[MarkusHarmsen]

I would like to leave a small note here. I did not test anything and so I may be totally wrong, but isn't any construct like

begin
  old = Klass.value
  Klass.value = new

  yield

ensure
  Klass.value = old
end

prone to race conditions? We had once the case (in a complete different context and not related to this gem) that the yield-part took roughly 10ms but under high load (on a threaded server,e.g. puma) a race condition occurred.

Example: 2 requests (r1 and r2) hit the server nearly at the same time. Now for r1 old is set to the current (correct) value, Klass.value will now be set to new and afterwards yield begins. Now r2 enters the stage, sets old to the current value (which is new)... When the r1 yield finishes, it resets Klass.value to the correct old value. But if now r2 yield finishes, Klass.value is set to the wrong new value.

Do you think this might occur here as well?

Edit: ok it seems that i18n is not threadsafe by itself ruby-i18n/i18n#269

[tagliala]

@MarkusHarmsen I'm not sure but I had several issues related to thread and to this gem when it used the before filter without the around block: take a look at #44

Unfortunately I removed that test repositories but I should be able to replicate it

I'm really interested in understand if there is a proper way to handle this.

Take also a look here: http://guides.rubyonrails.org/i18n.html#setting-and-passing-a-locale

[enriclluelles]

@MarkusHarmsen reading the i18n source code it seems it is thread safe, at least from my POV looking at this code: https://github.com/svenfuchs/i18n/blob/master/lib/i18n.rb#L22

What you said above is right:

begin
  old = Klass.value
  Klass.value = new

  yield

ensure
  Klass.value = old
end

allows for race conditions to restore the wrong value but in theory:

begin
  old = Klass.value
  Thread[:value] = new

  yield

ensure
  Thread[:value] = old
end

shouldn't. old is local so there's one per thread and value is thread scoped.

Anyway, this pr shouldn't have any effect regarding that so I'll merge it and add some more tests regarding thread safety to the gem

[MarkusHarmsen]

Your totally right! I18n.locale seems to be pseudo globally (per thread), so this is indeed not a problem.

Sorry to alarm everybody ;)

[tagliala]

Anyway, this pr shouldn't have any effect regarding that so I'll merge it and add some more tests regarding thread safety to the gem

it shouldn't because now it is doing the same thing.

I'm wondering if

def set_locale_from_url
      I18n.with_locale(params[RouteTranslator.locale_param_key] || RouteTranslator::Host.locale_from_host(request.host)) do
        yield
      end
    end

is better. I need to do a test with globalize, I will let you know

[gi-lunaweb]

Hi @tagliala

with_host_locale was written as a clone of I18n.with_locale that set the default locale based on host config. It aims to reuse most of existing mechanisms from I18n (but no I18n.with_default_locale exists).

As it already use the yield keyword instead of Proc.call, I don't really see the point of your patch unless reduce the call stack by 2 function calls. Does it really helps compared to the whole Rails stack and its myriads of methods ? You gave some stats from your apps but no references to compare with.

[tagliala]

I don't really see the point of your patch unless reduce the call stack by 2 function calls.

It is not only about the 2 function calls, it is also about calling functions making the same thing

The previous version was something like:

 def with_host_locale(&block)
      host_locale = RouteTranslator::Host.locale_from_host(request.host)
      begin
        if host_locale
          original_default         = I18n.default_locale
          original_locale          = I18n.locale

          I18n.default_locale = host_locale
          I18n.locale         = host_locale
        end

        def with_locale(tmp_locale = nil)
          if tmp_locale
            current_locale = self.locale
            self.locale    = tmp_locale
          end
          yield
        ensure
          self.locale = current_locale if tmp_locale
        end

      ensure
        I18n.default_locale = original_default if host_locale
        I18n.locale         = original_locale  if host_locale
      end
    end

I removed the unneeded extra work, it can't hurt

Does it really helps compared to the whole Rails stack and its myriads of methods ?

I usually don't think this way... I'm using this gem, I saw an easy way to improve the controller and I submitted a pull request...

If there are unoptimized pngs in a gem I use, I also make PRs to fix that even if few bytes are nothing compared to dozens of http requests

You gave some stats from your apps but no references to compare with.

Sorry, the above are only specs because I was checking for unintended side effects on my applications.

I'm new to profiling, I'm not confident in submitting test results.

[gi-lunaweb]

It is not only about the 2 function calls, it is also about calling functions making the same thing. I removed the unneeded extra work, it can't hurt.

Sorry, I didn't understand your patch this way. Based on your first comment, I tried to find why focus on some method calls and didn't see they did very similar things. Now it's clearer and, even if the performance gain is not obvious (I don't measure it and am not very confident to do it reliably), it can only be better.

Thanks for your work and explanations