Just load the theme negotiators which are needed

At least major, since #2429617-36: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) showed it to take >4% after the SmartCache patch is applied, roughly ~1% with HEAD.

First attempt, everything seems to work when testing manually, but the tests still need to be converted so will fail. Uploading the patch to get feedback on approach. Probably needs some polishing.

Initial numbers on performance impact:
=== 8.0.x..2471657 compared (552fb1c82db64..552fb30e6fe9a):
ct : 53,018|52,755|-263|-0.5%
wt : 185,132|183,287|-1,845|-1.0%
mu : 18,538,776|18,527,664|-11,112|-0.1%
pmu : 18,698,432|18,687,480|-10,952|-0.1%
---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

• Added a container compiler pass: RegisterThemeNegotiators, which finds all services tagged 'theme_negotiator' and ensures these are injected in the ThemeNegotiationSubscriber.
• ThemeNegotiationSubscriber alters routes when routes are rebuilt, adding an '_theme_negotiators'-option containing the prioritized theme negotiator service names that apply for that route.
• ThemeNegotiator now only evaluates the theme negotiators that are stored on the route.

Great job! Now to @dawehner for a review.

1. Removed the @TODO. During registration of the available theme negotiators, only the first 'theme_negotiator'-tag and its priority is taken into account. I don't see any reason to have multiple theme_negotiator tags with differing priorities on one service, so I think this is fine.
2. Done.
3. Done.

Tests still need to be converted. Since we transformed from having all logic in the ThemeNegotiators to splitting the logic up in three different locations ((a) container compiling, (b) route rebuilding and (c) theme negotiator) it might make sense to convert the current Theme Negotiator test from a Unit test to a Web Test?

New performance statistics:
=== 8.0.x..2471657 compared (5530c03d7e613..5530c300bd097):

ct : 53,018|52,752|-266|-0.5%
wt : 185,591|184,057|-1,534|-0.8%
mu : 18,537,112|18,525,848|-11,264|-0.1%
pmu : 18,696,824|18,685,744|-11,080|-0.1%

Unassigning since converting the tests from Unit tests to KernelTestBase has proven a bit too difficult for me...

I started out converting the ThemeNegotiatorTest from a UnitTest to a KernelTest. However, AFAICT, all test cases covered currently by ThemeNegotiatorTest are already covered by existing WebTestCases except for theme access checking.

The only thing not already explicitly covered is the theme access checking done in determineActiveTheme in the ThemeNegotiator, so I added an additional test case for that to ThemeTest.

Sounds good, great research!

Patch looks great already, just 2 test fails.

I like the approach of setting the theme negotiators in a onRouteAlter() and checking the applies() part.

That is really good thinking.

1000 auth requests ($ ab -n 1000 -c 1 -g ab.tsv -C SESS3d5f1d095f03cbeab065f280e5c42801=mK-rDsC8XPuc1zGUOsKOFDa5s8PvRVVB89XlO99pABk http://tres/)

Before

Requests per second:    12.66 [#/sec] (mean)
Time per request:       78.984 [ms] (mean)
Time per request:       78.984 [ms] (mean, across all concurrent requests)
Transfer rate:          163.45 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       1
Processing:    75   79   1.7     79      89
Waiting:       66   70   1.6     70      80
Total:         75   79   1.7     79      89

After

Requests per second:    12.74 [#/sec] (mean)
Time per request:       78.471 [ms] (mean)
Time per request:       78.471 [ms] (mean, across all concurrent requests)
Transfer rate:          164.52 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    74   78   2.0     78      95
Waiting:       66   70   1.9     69      84
Total:         74   78   2.0     78      95

Conclusion

Approximately a half a millisecond improvement, but tough to measure. So, I did profiling to find the function call/mem delta:

Number of Function Calls	40,660	40,531	-129	-0.3%
Incl. MemUse (bytes)	18,901,128	18,851,512	-49,616	-0.3%
Incl. PeakMemUse (bytes)	19,057,872	19,007,832	-50,040	-0.3%

(Reusing data from https://groups.drupal.org/node/464283#comment-1100578.)

The 2 remaining test failures were due to incorrect theme negotiation logic on route '<none>'. Tests should now pass.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ThemeNegotiatorSubscriber.php
   @@ -0,0 +1,65 @@
   +    foreach ($event->getRouteCollection() as $route_name => $route) {
   +      $route_negotiators = [];
   +      foreach ($all_negotiators as $service_id) {
   +        /** @var \Drupal\Core\Theme\ThemeNegotiator $theme_negotiator */
   +        $theme_negotiator = $this->container->get($service_id);
   +        if ($theme_negotiator->applies($route_name, $route)) {
   +          $route_negotiators[] = $service_id;
   +        }
   +      }
   

   So this runs all theme negotiators for all routes?

   How often is this done?

   Is there some kind of caching for the routes when the router is rebuild?

   How long does that take in cold cache?

2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   @@ -98,8 +66,16 @@ public function applies(RouteMatchInterface $route_match) {
   +      // theme negotiators.
   +      if ($route_theme_negotiator_ids || $negotiator->applies($route_match->getRouteName(), $route)) {
   

   This if looks wrong ...

   Why would we apply the determineActiveTheme if negotiators are stored on the route?

   What if there are several?

   I see this tries to optimize for the case that there is only one in which case we know it is matching, but for that it should check for count($r_t_n_ids) == 1.

Thanks for the review Fabianx

Regarding #1:

• This indeed runs all theme negotiators for all routes. Note that theme negotiation logic is fairly simple in and of itself, although I assume that on a site with a large number of routes and a large number of theme negotiators, this can result in a significant number of function calls.
• This theme negotiator evaluation happens every time routes are rebuilt. This is in essence the change from the current situation, where all theme negotiators are instantiated and evaluated per request.
• I don't know if there is any caching when routes are rebuilt...

I'm happy to profile impact of this patch on cold cache requests if someone can tell me how to properly test this (I have xprof and xhprof-kit already up-and-running, just need to know how this is usually tested/benchmarked for D8 core).

Regarding #2:
There are two possible scenarios when determineActiveTheme() is called: either (a) the theme negotiators that apply to the route have been stored on the route in \Drupal\Core\EventSubscriber\ThemeNegotiatorSubscriber::onRouteAlter(), in which case the service ids stored on the route are used, or (b) there are no theme negotiators stored on the route, in which case we need to evaluate the full set of available theme negotiators.

In scenario a), we just need to cycle through the services stored on the route until the determineActiveTheme for that negotiator returns a theme. However, in scenario b), we need to first test if the negotiator applies to the route by calling applies() (contrary to the route-stored negotiators, where this step happened earlier). Only if the negotiator applies, we should call determineActiveTheme().

Hence if ($route_theme_negotiator_ids || $negotiator->applies($route_match->getRouteName(), $route)) means "determine the active theme for this negotiator, but if it was not stored on the route, make sure it applies first".

#21

1: Thanks for explanation, so we should maybe add a threshold on the route collection OR just rely on sites to remove the subscriber from services if number of routes is too high?

2. Thanks, again makes perfect sense now. I missed that they are sorted ...

-------------------------

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterThemeNegotiators.php
   @@ -0,0 +1,33 @@
   +    foreach ($container->findTaggedServiceIds('theme_negotiator') as $id => $attributes) {
   +      $service_ids[$id] = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
   +    }
   +
   +    $container
   +      ->getDefinition('theme.negotiator')
   +      ->addArgument($service_ids);
   

   Who sorts this negotiators now?

   I think this is missing now ...

   --

   Do we use addArgument elsewhere?

   It seems way underdocumented as you cannot see there is another parameter from the core.services.yml.

   I think its more logical to have a dedicated setNegotiatorServiceIds() function ...

   But I might be wrong.

   Probably just a arsort thats needed, then using array_keys ...

   EDIT: Found it, see below ...

2. +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
   @@ -71,9 +72,9 @@ public function __construct(CsrfTokenGenerator $token_generator, ConfigFactoryIn
   -  public function applies(RouteMatchInterface $route_match) {
   +  public function applies($route_name = '', Route $route = NULL) {
   

   Not sure we can do this API change this late, but need to at least document the API change.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   @@ -49,48 +45,20 @@ class ThemeNegotiator implements ThemeNegotiatorInterface {
   +    arsort($negotiator_service_ids);
   +    $this->negotiator_service_ids = array_keys($negotiator_service_ids);
   

   Ahhh, here we are doing the sorting, okay ...

   That makes sense.

4. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   @@ -98,8 +66,16 @@ public function applies(RouteMatchInterface $route_match) {
   +    $route = $route_match->getRouteObject() ?: NULL;
   +    $route_theme_negotiator_ids = ($route && $route->getOption('_theme_negotiators')) ? $route->getOption('_theme_negotiators') : array();
   

   So in case someone removes the subscriber this is exactly the old behavior?

   Nice!

5. +++ /dev/null
   @@ -1,188 +0,0 @@
   - * @file
   - * Contains \Drupal\Core\Theme\ThemeNegotiatorTest.
   - */
   

   Lets keep that fully, this can still test the case when no negotiators are set at all on routes (as I have too many on my site and don't want the optimization).

@mr.baileys are @Fabianx's notes doable and is this doable for RC or should we postpone till 8.1.x?

I don't see this happening now (between RC4 and 8.0.0). But maybe it can be done in 8.0.1? If there's no API change whatsoever, it's possible.

Potentially clashes with #2292217: ThemeNegotiator::determineActiveTheme() should not require a RouteMatch to be passed in.