- Issue created by @berdir
- 🇬🇧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 → (Closed) created by berdir
- 🇨ðŸ‡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)
- 🇬🇧United Kingdom catch
Looks like we could move some logic around in AdminNegotiator to do the cheapest checks first at least.
- 🇨ðŸ‡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.
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
I think this change on it's own is a good thing already, and followups can be created to make changes in AdminNegotiator?
- 🇨ðŸ‡Switzerland berdir Switzerland
Agreed, 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). 📌 Aggregate cache operations per bin to allow for more specific asserts Active 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.
- 🇬🇧United Kingdom catch
Will be nice to see the diff in 📌 Aggregate cache operations per bin to allow for more specific asserts Active but I don't think that issue should block this one.
Opened 📌 Try to simplify checks in AdminNegotiator Active .
Committed/pushed to 11.x, thanks!
Automatically closed - issue fixed for 2 weeks with no activity.
