The list of extensions is stored in the default cache bin. It is however required very early, due to the current theme cache context on dynamic page cache.
I noticed this because of #3498940: Optimize bin cache tags and last write timetamp. Between the cache get of the dynamic page cache redirect and the actual dynamic page cache, there was also the cache get of "core.extension.list.theme".
That's because ThemeNegotiator needs to check if the theme is enabled and for that, it needs to load all the themes.
#1 /var/www/html/modules/contrib/redis/src/Cache/CacheBase.php(384): unknown()
#2 /var/www/html/modules/contrib/redis/src/Cache/PhpRedis.php(68): Drupal\redis\Cache\CacheBase->expandEntry()
#3 /var/www/html/modules/contrib/redis/src/Cache/CacheBase.php(159): Drupal\redis\Cache\PhpRedis->getMultiple()
#4 /var/www/html/core/lib/Drupal/Core/Extension/ExtensionList.php(280): Drupal\redis\Cache\CacheBase->get()
#5 /var/www/html/core/lib/Drupal/Core/Extension/ThemeHandler.php(72): Drupal\Core\Extension\ExtensionList->getList()
#6 /var/www/html/core/lib/Drupal/Core/Theme/ThemeAccessCheck.php(55): Drupal\Core\Extension\ThemeHandler->listInfo()
#7 /var/www/html/core/lib/Drupal/Core/Theme/ThemeNegotiator.php(69): Drupal\Core\Theme\ThemeAccessCheck->checkAccess()
#8 /var/www/html/core/lib/Drupal/Core/Theme/ThemeManager.php(407): Drupal\Core\Theme\ThemeNegotiator->determineActiveTheme()
#9 /var/www/html/core/lib/Drupal/Core/Theme/ThemeManager.php(95): Drupal\Core\Theme\ThemeManager->initTheme()
#10 /var/www/html/core/lib/Drupal/Core/Cache/Context/ThemeCacheContext.php(43): Drupal\Core\Theme\ThemeManager->getActiveTheme()
#11 /var/www/html/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php(123): Drupal\Core\Cache\Context\ThemeCacheContext->getContext()
#12 /var/www/html/core/lib/Drupal/Core/Cache/VariationCache.php(286): Drupal\Core\Cache\Context\CacheContextsManager->convertTokensToKeys()
#13 /var/www/html/core/lib/Drupal/Core/Cache/VariationCache.php(221): Drupal\Core\Cache\VariationCache->createCacheIdFast()
#14 /var/www/html/core/lib/Drupal/Core/Cache/VariationCache.php(35): Drupal\Core\Cache\VariationCache->getRedirectChain()
#15 /var/www/html/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php(142): Drupal\Core\Cache\VariationCache->get()
#16 /var/www/html/vendor/symfony/event-dispatcher/EventDispatcher.php(246): Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber->onRequest()
(I like how VariationCache is very optimistic here with createCacheId*Fast* ;))
Can we move this cache to the bootstrap bin? There might be quite a few themes.
Somewhat related, this doesn't just determine the active theme, it also initializes and loads it. We don't really have a way to not do that, and I don't know if that really can be avoided in case of a dynamic page cache hit and what happens afterwards, but might be worth to investigate?
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !10956
Comments
Comment #2
catchI think it definitely makes sense to put this in the bootstrap bin, it's called on every html request.
Comment #4
berdirHad a quick look but that seems like the only feasible quick improvement. Even when there is a dynamic page cache hit, there's a very high chance that there's at least one placeholder to be rendered, either bigpipe or initially, both need both the theme extension list (twig render loads that) and the initialized theme, obviously.
There's definitely stuff to untangle between ThemeManager, ThemeInitialization and ThemeExtensionList, all do a bit of loading, and it seems sensible that we'd have an ability to only load and cache enabled themes. But there are far fewer themes on a site in most cases compared to modules, so that's all a problem for another day.
Changing the cache bin is enough to remove the extra cache get on default bin:
(The entity_types cache tag is also through theme negotiation, because AdminNegotiator checks user roles storage (?), seems strange, but the best we can hope for is shift that around. And if we stop persistent caching for access policies in core then it would move there)
Comment #5
catchLooks like we could move some logic around in AdminNegotiator to do the cheapest checks first at least.
Comment #6
berdirMaybe, but as mentioned, it's moving up to access policy with #3500683: Allow access policies to opt out of caching and then it will be cheap.
FWIW, I have no idea why that whole check exists. There is no possibility that user roles don't have a storage handler? This check was added all the way back in #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system in 2013 when AdminNegotiator was introduced, there was no similar thing before that in the code that it replaced.
Comment #7
borisson_I think this change on it's own is a good thing already, and followups can be created to make changes in AdminNegotiator?
Comment #8
berdirAgreed, the only thing IMHO is that I wish this improvement would actually be visible on our performance tests (I assume the actual grafana/metrics _should_show one query less on those requests). #3500739: Aggregate cache operations per bin in performance tests to allow for more specific asserts will make it more visible, but I can also update that MR once this lands and the improvements will be visible there in a commit on the MR.
Comment #10
catchWill be nice to see the diff in #3500739: Aggregate cache operations per bin in performance tests to allow for more specific asserts but I don't think that issue should block this one.
Opened #3501727: Try to simplify checks in AdminNegotiator.
Committed/pushed to 11.x, thanks!