Many unecessary calls to t() for strings that are never shown anywhere

Note that we may be able to get t() faster by:
- Compiling the DIC: #1706064: Compile the Injection Container to disk.
- Creating a Translation class/service that depends on the language manager service (so it's auto-reinitialized in subrequests) that could get the interface language in its constructor rather each time t() is called.
- Add a translateMultiple() method to that class, so we can do a bunch of strings in one function call.

However, I think this issue makes sense regardless. Calling t() on something and then throwing away the result seems like an inefficiency worth cleaning up.

Ha!

system_region_list actually has only one t() function.

function system_region_list($theme_key, $show = REGIONS_ALL) {
  $themes = list_themes();
  if (!isset($themes[$theme_key])) {
    return array();
  }

  $list = array();
  $info = $themes[$theme_key]->info;
  // If requested, suppress hidden regions. See block_admin_display_form().
  foreach ($info['regions'] as $name => $label) {
    if ($show == REGIONS_ALL || !isset($info['regions_hidden']) || !in_array($name, $info['regions_hidden'])) {
      $list[$name] = t($label);
    }
  }

  return $list;
}

Do we REALLY need region names to be translated? And if we do, can't we just use the value that is defined by the theme. If you want it translated change it there. It's YOUR theme after all.

Indeed we want to translate region names, but in administration screens only, system_region_list() is called on every page and not even cached, which means that modules using it will trigger those t() calls (even strings are cached, it's not really a good practice).

I don't like having boolean parameters everywhere, it's not a really good practice, I'd prefer the 2 functions approach instead: one of them being a pure utility function for rendering or guessing regions, without any human strings returned.

Yes of course, but as a good practice I think it's good to remove unnecessary calls whenever they happen (t or any other function).

Hm. I guess that my proposal from some time ago would actually fix this:

#1213510: A modern t()

The idea is to defer the actual translation to the time when a string is actually printed - until then, the original string including arguments and context remains available as a very slim and simple object (which even allows people to alter it, fwiw).

I don't think so, it just would change the t() function to some other mecanism, but it wouldn't eradicate unnecessary t() calls.

IMO, performance optimizations like these would be best considered after Feature freeze.

I've been toying with an idea for deferred translation with static caching. Patch if (very) far from passing, but the idea might be onto something by deferring translation as much as feasible, so I'm putting it up before forgetting about it.

Basically, the idea is to make t() return an untranslated object, and only perform the translation when the result is actually used as a string, be it for a cast, append or whatever operation requiring a string.

Translations are currently kept in a static cache in case they can be reused. This would probably be more efficiently located in locale, possibly in the LocaleLookup class, which already performs translation static caching.

Checking the kind of errors the code returns shows that in some places we tend to make not-really-warranted assumptions like "if this is an object, it has a $option property" (_form_options_flatten(), form_select_options()), or "it is safe to invoke strpos() on an untyped variable (WebTestBase::assert[Raw|NoRaw|TextHelper]()), which might be worth changing regardless of this issue,

In the same vein, the apparently useless implement of printed() and render() are needed because some code, notably in Template, assumes that any object it receives has such a method, and the apparently useless ArrayAccess implementation is needed because of similar assumptions that any non-string is an array (ajax handling in forms).

Per #9 and #10, how about moving #12 to #1213510: A modern t()? I like the spirit of it, and possibly if we get that in, we'll find that t() itself becomes trivially fast enough that this issue becomes low priority, but I think it's still a separate issue.

Berdir++

This looks great.

1) I wonder whether it would make sense to have system_region_list() return the region names as both keys and values, so we could use the much faster isset() instead of in_array() in various places?

2) I guess this patch will break #1535868: Convert all blocks into plugins — so I wonder whether we want to postpone it on that?

If this is shown 51 times for an anon user on the front page, that warrants a static cache in system_region_list(), right?! That's probably a follow-up issue, but I wanted to ask in case I got something terribly wrong.

@#12 I don't like the idea of having a lazy init t object: t backend is supposed to do itself caching, if it needs to (systems such as the PHP gettext library don't need it at all, since the compiled mo files are in PHP shared memory anyway, just like APC bytecode or user cache). Doing this introduces a second caching layer, and it's getting closer to potential cache bloats.

I really prefer no caching at all, reducing the unnecessary call in a pure algorightmic manner, then once done and if the modern t patch comes to agreement, let the backends handle their own cache to avoid double, triple, quadriple caching (real cache bloat). Let's not forget that today Drupal specific locale layer already does a very aggressive caching and uncessary t calls are not costy because it needs to fetch data, but because there is too many of them.

Drupal is almost subject to cache bloats already at some various points, let's not make this even more serious than it is.

EDIT: We have to reduce algorithmic complexity at the very best we can. Optimization can come later, but must not come before, because we will introduce unnecessary duplicated caching layers and algorithmic complexity instead of trusting the layers below (OS cache, PHP cache, APC cache, anything cache below us, which will be a lot more efficient at the end of the day) and the upper layer (smart algorithm, function signature and software design homogeneity, etc..., trust the developers themselves for writing good features and using correctly our API's).

@pounard thanks for looking into it, at least. Your observations do make perfect sense.

@Berdir: Right, that makes sense. I thought the function itself gets called 51 times. Thanks for the explanation.

Looks good to me ...

Rethinking my #12 patch and @pounard's observations, this is not so much about caching than about lazy evaluation, actually.

But this bears no relationship with Berdir's patch.

I am all in for lazy instanciation, but I don't want it to make code more complex than it is. I'm thinking about pluggability among all other things: if the we want to make the locale system pluggable, we cannot rely easily on such lazy mecanism because the implementation provider may consider than getting the string could be faster than the complex lazy init mecanism.

Just an a example I'm attaching a D7 patch I used on a project (which seem to work, not always, but in most cases) and that saved a lot of time in some pages (for example, a Commerce site with a complex front page can go up to 3,000 t() calls, using this patch I ended up with only 100). Please consider I'm absolutely not pleased with this method and patch that I consider as an ugly workaround.

EDIT: Ignore this patch tests.

I'd be happy to reroll #17 if folks think that's still the way to go on this.

Looks like the system_region_list() part of this was handled for Drupal 8 in #2497259: system_region_list() unnecessarily translates region names.

anything left here?

The part with stream wrappers as mentioned in the issue description and maybe others.