Contrib.social

Review cache bin usage of ThemeExtensionList and Theme cache context

Open on Drupal.org →
Created on 18 January 2025, 3 days ago

Problem/Motivation

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 📌 Optimize bin cache tags and last write timetamp Active . 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* ;))

Steps to reproduce

Proposed resolution

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?

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component

theme system

Created by

🇨🇭Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @berdir
  • Comment 3 days ago →
    🇬🇧United Kingdom catch

    I think it definitely makes sense to put this in the bootstrap bin, it's called on every html request.

  • Merge request !10956ExtensionListTheme should use bootstrap cache as it's used on every request → (Open) created by berdir
  • Comment 3 days ago →
    🇨🇭Switzerland berdir Switzerland

    Had 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:

    "HGETALL" "core:dynamic_page_cache:response:[request_format]=html:[route]=view.fro....

"GET" "core:cachetags:x-redis-bin:dynamic_page_cache"
"GET" "core:dynamic_page_cache:_redis_last_delete_all"
"GET" "core:cachetags:entity_types"
"HGETALL" "core:dynamic_page_cache:response:[cookies:big_pipe_nojs]=:[languages:la....

    (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)

  • Pipeline finished with Success
    3 days ago
    Total: 2608s
    #399982
  • Comment 2 days ago →
    🇬🇧United Kingdom catch

    Looks like we could move some logic around in AdminNegotiator to do the cheapest checks first at least.

  • Comment 2 days ago →
    🇨🇭Switzerland berdir Switzerland

    Maybe, but as mentioned, it's moving up to access policy with 📌 Review cache bin and cache tags of access policy caching Active 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.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024