Improve the speed of \Drupal\Core\Theme\ThemeAccessCheck

Is there a patch to review?

Looks great, just need to fix the test failures.

The problem with the tests is because of the architecture of ThemeInitialization::getActiveThemeByName(). When ThemeInitialization::getActiveThemeByName() tries to return a theme that is not installed he calls ThemeInitialization::getActiveTheme() with a fake theme called core. From my point of view this a bad design but this is not subject of this issue.

Forgot the interdiff.

New issue created #2545192: Don't assign 'core' to a theme that cannot be negotiated or is not in the installed theme list.

Added some docs as you suggested in #9.

Ouch!

Accidentally I added the same patch twice in #10. Back to NR.

@dawehner, can you point me to some docs, best practices, writeups on how to provide an acceptable benchmark?

I'm afraid this is a dead end :(

EDIT: tested with 1000 requests to /contact, as the anon users, with page cache off.

Before

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

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    68   71   1.2     71      76
Waiting:       60   64   1.2     63      69
Total:         68   71   1.2     71      76

Percentage of the requests served within a certain time (ms)
  50%     71
  66%     71
  75%     71
  80%     72
  90%     72
  95%     73
  98%     74
  99%     75
 100%     76 (longest request)

After

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

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    67   72   1.3     71      78
Waiting:       60   64   1.3     64      71
Total:         67   72   1.3     72      78

Percentage of the requests served within a certain time (ms)
  50%     72
  66%     72
  75%     72
  80%     73
  90%     73
  95%     74
  98%     75
  99%     76
 100%     78 (longest request)

Here's what I got with the /contact as the front page. xdebug and all caching turned off. Twig debug turned off and twig caching still on.

Also I enabled all the modules and a few contrib modules too but still accessing that page anonymous so it shouldn't make I difference I'd expect.

=== 8.0.x..8.0.x compared (5659123816918..565914310b034):

ct  : 170,827|170,827|0|0.0%
wt  : 632,030|633,399|1,369|0.2%
mu  : 33,371,040|33,373,152|2,112|0.0%
pmu : 35,105,000|35,107,632|2,632|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5659123816918&...

=== 8.0.x..themeaccesscheck compared (5659123816918..5659177493c27):

ct  : 170,827|170,851|24|0.0%
wt  : 632,030|640,893|8,863|1.4%
mu  : 33,371,040|33,387,336|16,296|0.0%
pmu : 35,105,000|35,121,464|16,464|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5659123816918&...

=== SUM: 8_0_x-summary..themeaccesscheck-summary compared (565918537d922..5659186b7d6e0):

ct  : 17,091,157|17,093,710|2,553|0.0%
wt  : 69,237,937|69,699,950|462,013|0.7%
mu  : 3,337,669,336|3,339,222,864|1,553,528|0.0%
pmu : 3,511,345,104|3,512,874,752|1,529,648|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=565918537d922&... 8_0_x-summary..themeaccesscheck-summary

@claudiu.cristea is there something else we could test directly because page hits aren't significantly different it seems from the tests above.

Setting to NW for update to profiling scenario/instructions.

This is still valid, but we have a container parameter with a list of themes now, so I can't see a reason not to use that.

Attaching xhprof (xhgui) screenshots. This appears to save about 1ms from each request, the call chain is via the theme cache context.

One of the Nightwatch tests consistently fails while trying to install a theme, so I wonder if this has a subtle side effect somewhere?

The nightwatch failure is because the nightwatch install theme command relies on a controller that changes the default/admin theme in a GET request then immediately renders a response - the active theme is determined after the new theme is installed, but the ThemeAccess service isn't refreshed with the new container parameter afaik.

I can't see any obvious way to force this to be refreshed, or even to workaround it in the test, so for now removed the dependency injection so that the list of themes is got from the container at the very last minute.

I wonder that if #3481903: Support hooks (Hook attribute) in any registered service can land (though there are some challenges), then services can implement hook_modules_installed() and/or hook_themes_installed() and refresh their properties from the new container there.

We don't need an entire support module to handle changing the theme for exactly three Nightwatch tests. MR!15405 removes the module and just uses /admin/appearance to change the theme.

Claude Code wrote the change to drupalEnableTheme.js.

OK better explanation (mostly in a code comment now).

Access checkers all get constructed when the event dispatcher needs them - this happens before the controller method is called, e.g. when checking access to the route or similar.

The controller method installs a theme then immediately sets it as the admin theme - and the controller is an admin page.

Theme negotiation happens after the method is called, but the access check service has already been constructed before the controller method is called. This means that the theme isn't in the list of themes passed to the access checker.

This all means we can workaround the bug in the nightwatch test module, I find it very unlikely that any equivalent code exists anywhere else.

There are probably cleaner ways to fix it though:

1. In the theme negotiatior, instead of injecting the access checker service, we could inline the logic - the actual logic is one line. This assumes the theme negotiator will be fresh from the container, which it might not.

2. To be honest I don't understand the point of this access check during theme negotiation altogether - how does an uninstalled theme get passed in the first place? Going to open an issue to look into removing it.

Still had the tab open and crossposted with #43.

Dropping the support module seems fine too, although I managed to get things isolated to the support module now so that could also be a follow-up. We have two issues open to replace these nightwatch tests anyway.

Going to open the follow-up to see whether the theme access check is really necessary too.

Opened #3584064: Check whether ThemeAccess check during theme negotiation is necessary.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

How do you suggest we proceed with this in light of #3584064: Check whether ThemeAccess check during theme negotiation is necessary? if we remove the need for it entirely or put it in an asser(), do we still do this too?

We need to figure out whether we need bc or not in that issue, so for me I'd go ahead here since it's a simple change, then keep going over there. If we go with an assert() in the other issue, it would be better to assert the simple check added here rather than the logic in HEAD too.

After thinking about it, I think it makes sense to do this as well. Especially if we only make it an assert() then it would mean that performance tests would have different results depending on whether or not assert is enabled and profiling is often done with assert on and then could lead people down a wrong path.

Added one question about BC on the constructor. I'd suggest doing this, not really because of possible subclasses but more so for handling updates with an existing container. This would probably break pretty hard when trying to access the site without a cache clear.

I don't think we need to provide bc for subclasses, but handling the stale container sounds good. While core updates usually for a container rebuild immediately via the container cache key, some sites set manual deployment identifiers and might forget. Added a commit for that.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thanks. For constructor parameters with BC, I prefer not promoting them directly, this means we can't set the type on that because of BC, but I think it's OK either way.

Yeah I went for 'least amount of change', technically we could use a union type in the constructor, but then the property itself would have the union type so not sure that's much better either. Given the constructor is internal and so is the class (even though it's injected in core, it's a tagged service really) not sure it matters much.

I've hidden @longwave's branch with the nightwatch refactor but I added a note on #3553673: Migrate from Nightwatch to Playwright (and phpunit Axe) tests that the playwright conversion should tackle it.

Made two suggestions and applied them myself because they were minor changes to comments.
Committed and pushed 7a031e3 to main and d3126db to 11.x. Thanks!