~50 calls to t() for two strings in LanguageManager() on every request

I don't think we can move it to class variables, unless we initialize them in the constructor? That sounds fine with me. For later translation, we can use a TranslationWrapper, but not sure that would come out more performant?

Here's a patch.

Looks great, assigning to Gábor for final review.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -91,18 +91,18 @@ public function getLanguageTypes() {
         'name' => $this->t('Content'),
...
       LanguageInterface::TYPE_URL => array(

Why not change the names for content and URL and the description for URL?

Wait, URL does not have a name or description here. But as per the comment you modify it should because language_language_type_info() has those.

Hmmm... but we have code that checks the result of this and looks for name and does different things... /me is confused.

I think the only place was in views and we don't do that anymore but tie to locked there too.

@Gábor Hojtsy afiacs we have 3 places in core where we are checking for the name.

  /**
   * {@inheritdoc}
   */
  public function onBlockActiveContext(BlockContextEvent $event) {
    // Add a context for each language type.
    $language_types = $this->languageManager->getLanguageTypes();
    $info = $this->languageManager->getDefinedLanguageTypesInfo();
    foreach ($language_types as $type_key) {
      if (isset($info[$type_key]['name'])) {
        $context = new Context(new ContextDefinition('language', $info[$type_key]['name']));
        $context->setContextValue($this->languageManager->getCurrentLanguage($type_key));
        $event->setContext('language.' . $type_key, $context);
      }
    }
  }

      // We only go through the configured types.
      foreach ($types as $id) {
        if (isset($types_info[$id]['name'])) {
          $name = $types_info[$id]['name'];
          // Surround IDs by '***LANGUAGE_...***', to avoid query collisions.
          $id = '***LANGUAGE_' . $id . '***';
          $list[$id] = $this->t('!type language selected for page', array('!type' => $name));
        }
      }
      if (!empty($current_values)) {
        foreach ($types_info as $id => $type) {
          $id = '***LANGUAGE_' . $id . '***';
          // If this (non-configurable) type is among the current values,
          // add that option too, so it is not lost. If not among the current
          // values, skip displaying it to avoid user confusion.
          if (isset($type['name']) && !isset($list[$id]) && in_array($id, $current_values)) {
            $list[$id] = $this->t('!type language selected for page', array('!type' => $type['name']));
          }
        }
      }

in Drupal\views\Plugin\views\PluginBase

  public static function queryLanguageSubstitutions() {
    $changes = array();
    $manager = \Drupal::languageManager();

    // Handle default language.
    $default = $manager->getDefaultLanguage()->getId();
    $changes[PluginBase::VIEWS_QUERY_LANGUAGE_SITE_DEFAULT] = $default;

    // Handle negotiated languages.
    $types = $manager->getDefinedLanguageTypesInfo();
    foreach ($types as $id => $type) {
      if (isset($type['name'])) {
        $changes['***LANGUAGE_' . $id . '***'] = $manager->getCurrentLanguage($id)->getId();
      }
    }

    return $changes;
  }

Oh, so ConfigurableLanguageManager::getLanguageTypes() only returns configurable types, which both the block code is based on and the views plugin base. The query replacements are just query replacements, so having non-configurable options there does not cause problems. So looks like LanguageManager::getDefinedLanguageTypesInfo() should not have URL label and description indeed, because LanguageManager::getLanguageTypes() returns all types, not just the configurable ones. (or we should make LanguageManager::getLanguageTypes() only return the configurable ones).

So looks like the comment needs fixing because its not true. We can choose to forego implementing the hook in language and just hardcode calling the parent in ConfigurableLanguageManager() for collecting the defaults and call the hooks afterward. The content language title would still need to be wrapped in TranslationWrapper.

Ping, so #11 makes totally sense. Do you plan to work on it? I think these two steps could be done pretty quickly.

Not planning to work on it. I think we can skip implementing the hook in language module too and call the parent, that would make us not required to do the same changes at two places and keep them consistent, which is a PITA.

So this issue itself saves 2,8ms for me which is ~2,5% of the entire request for the default frontpage anonymous, without page cache.
On the slower computer of catch we might get to > 5ms easily, which 5% of the default frontpage.

IMHO this is at least major then.

When catch and myself talked a bit more we realized what else could be skipped:

• With locale module enabled we could skip the locale cache on full cached requests
• When we don't call to t() we could make the translation wrapper actually lazy
• Once #2507031: Optimize automatic cron subscriber by moving automatic cron to a module is with point 2) and 1) this could lead to > 10ms on warm cached requests,
  so while this issue itself does NOT save 10ms, it will enable further optimizations.

I pushed too much because of stupid rules.

Opened a follow up: #2537042: [meta] Make t() more lazy and added some tasks in the issue summary.

That wasn't me, sorry

blub.

Removed the hook from language module, and get the default information from languageManager, as suggested in #13. Also wrapped the content language title in TranslationWrapper as suggested in #11.

I didn't run all the tests locally but ConfigLanguageOverrideWebTest and ConfigLanguageOverrideTest do work. So getting the defaults without the hook seems to work.
Let's see what the rest of the testsuite thinks.

Looks like all of the fails are because when the language module isn't enabled, it can't load the METHOD_ID constant on LanguageNegotiationUI / LanguageNegotiationUrl / LanguageNegotiationUrlFallback.

I could move those constants to the LanguageManager, but that doesn't feel like a correct solution.

After discussing this in IRC with dawehner I've set the fixed property from the language module so not use the constants in the core class.

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -87,22 +87,35 @@ public function getLanguageTypes() {
   +    return $this->definedLanguageTypesInfo = array(
   

   Assignment and returning in a single line is fairly standard in core. Can we put the return on a new line after the rest?

   Also, it seems since we're already storing the variable it would make sense to early return here as well, not just in the child implementation?

2. +++ b/core/modules/language/language.module
   @@ -283,42 +283,6 @@ function language_get_default_langcode($entity_type, $bundle) {
   -function language_language_types_info() {
   
   @@ -540,3 +504,16 @@ function language_tour_tips_alter(array &$tour_tips, EntityInterface $entity) {
   +function language_language_types_info_alter(array &$language_types) {
   

   I like that we're no longer duplicating information. Yay!

   I still think we should implement the info hook (like it is now), not the alter hook. Because ModuleHandler::invokeAll() does a recursive merge this works perfectly and is more in-line with our "build-then-alter" pattern, because here we just adding information not altering existing information.

3. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
   @@ -181,11 +181,16 @@ protected function loadLanguageTypesConfiguration() {
   +    // Gets the default implementation from the parent class.
   +    $defaults = parent::getDefinedLanguageTypesInfo();
   +
        if (!isset($this->languageTypesInfo)) {
   

   Seems this could move inside of the if now?

Fixed 1 and 3 out of #28. Not sure about 2.
I think this is what @Gábor Hojtsy meant in #11/ #13.

Ahh yes, sorry #28.2 in fact will not work just like this, because here we are manually merging the arrays with + ourselves, so that would have to replaced with a deep merge for my proposal to work. (I still think that makes sense because it is more inline with ModuleHandlerInterface::invokeAll(), but it will not just work to use the info hook instead of the alter)

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -87,28 +87,43 @@ public function getLanguageTypes() {
   +   * - Content language is by default non-configurable and inherits the interface
   

   Nitpick: goes over 80 columns

2. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -87,28 +87,43 @@ public function getLanguageTypes() {
        // This needs to have the same return value as
   -    // language_language_type_info(), so that even if the Language module is
   -    // not defined, users of this information, such as the Views module, can
   -    // access names and descriptions of the default language types.
   -    return array(
   +    // language_language_type_info(), so that even if the Language module is not
   +    // defined, users of this information, such as the Views module, can access
   +    // names and descriptions of the default language types.
   

   Not true anymore (and the function name was incorrect anyway).

3. +++ b/core/modules/language/language.module
   @@ -283,42 +283,6 @@ function language_get_default_langcode($entity_type, $bundle) {
   -function language_language_types_info() {
   

   Yay :)

4. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
   @@ -181,11 +181,17 @@ protected function loadLanguageTypesConfiguration() {
      public function getDefinedLanguageTypesInfo() {
   +
   

   Should not add that newline IMHO.

5. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
   @@ -181,11 +181,17 @@ protected function loadLanguageTypesConfiguration() {
   +      // Gets the default implementation from the parent class.
   +      $defaults = parent::getDefinedLanguageTypesInfo();
   

   Comment does not add value.

Fixes comments from #31

Looks good, makes sense, except:

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -87,28 +87,40 @@ public function getLanguageTypes() {
   -   * {@inheritdoc}
   +   * Returns information about all defined language types.
   +   *
   +   * Defines the three core language types:
   +   * - Interface language is the only configurable language type in core. It is
   +   *   used by t() as the default language if none is specified.
   +   * - Content language is by default non-configurable and inherits the
   +   *   interface language negotiated value. It is used by the Field API to
   +   *   determine the display language for fields if no explicit value is
   +   *   specified.
   +   * - URL language is by default non-configurable and is determined through the
   +   *   URL language negotiation method or the URL fallback language negotiation
   +   *   method if no language can be detected. It is used by l() as the default
   +   *   language if none is specified.
   +   *
   +   * @return array
   

   Does not explain the array structure so it needs to use somehow also inheritdoc, no?

2. +++ b/core/modules/language/language.module
   @@ -540,3 +504,16 @@ function language_tour_tips_alter(array &$tour_tips, EntityInterface $entity) {
   +/**
   + * Implements hook_language_types_info_alter().
   + *
   + * We can't set the fixed properties in \Drupal\Core\Language\LanguageManager,
   + * where the rest of the properties for the default language types are defined.
   + * The LanguageNegation classes are only loaded when the language module is
   + * enabled and we can't be sure of that in the LanguageManager.
   + */
   +function language_language_types_info_alter(array &$language_types) {
   

   Nice!

Are we sure we fixed the performance issue here?

Anonymous frontpage, no page cache. This certainly saves a bit.
When I look at an invidual providing you can't see any translate() call anymore in there.

=== 8.0.x..8.0.x compared (55b0fd184a943..55b0fd6da24bf):

ct  : 27,918|27,918|0|0.0%
wt  : 63,861|63,927|66|0.1%
mu  : 14,374,624|14,374,624|0|0.0%
pmu : 14,441,144|14,441,144|0|0.0%

=== 8.0.x..2497275 compared (55b0fd184a943..55b0fd7af1a1a):

ct  : 27,918|27,678|-240|-0.9%
wt  : 63,861|63,423|-438|-0.7%
mu  : 14,374,624|14,374,696|72|0.0%
pmu : 14,441,144|14,441,328|184|0.0%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2497275           |     27,678 |     29,365 |     27,695 |     27,678 |     27,678 |
| 8_0_x             |     27,918 |     29,605 |     27,935 |     27,918 |     27,918 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2497275           |     63,423 |     88,364 |     67,605 |     66,935 |     72,140 |
| 8_0_x             |     63,927 |     87,677 |     67,349 |     66,394 |     72,978 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2497275           | 14,374,696 | 14,783,408 | 14,414,017 | 14,374,696 | 14,726,064 |
| 8_0_x             | 14,374,448 | 14,783,376 | 14,423,402 | 14,374,624 | 14,715,212 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2497275           | 14,441,312 | 14,915,344 | 14,481,411 | 14,441,328 | 14,791,240 |
| 8_0_x             | 14,440,808 | 14,915,288 | 14,490,604 | 14,441,144 | 14,779,950 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

Regarding #33, we're sure this fixes the performance issue, @dawehner profiled this in #35.

Also changed the comment of 33.1 so it describes what the return array looks like. The entire wording that {@inheritdoc} would give us isn't correct in this case.

I think this is ready to fly now

+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -182,10 +182,14 @@ protected function loadLanguageTypesConfiguration() {
+      $this->moduleHandler->alter('language_types_info', $language_info);

We need to document this new hook.

I talk bullshit.

Superb, thanks! Great cleanup also!