Optimize CacheContextsManager:::convertTokensToKeys()/optimizeTokens()

Created on 28 June 2024, 2 months ago
Updated 16 July 2024, about 2 months ago

Problem/Motivation

It's hard to reliable guess what the real cost of it is, but I'm seeing it show up a lot in profiling.

Currently looking at a profile with 1k calls to it and 3% total time. Not much, but it adds up and might be a fairly easy target for optimization, at least in certain cases.

The site in question has large menu structures, mostly pointing to nodes in different groups, so a cache miss on the menu blocks has to load _many_ nodes and check access to them.

In my specific case, it's through group/flexible_permissions and its usage of VariationCache for that. It has a static and a persistent cache, that it calls many times with the same arguments (a few variations) and specifically a single cache context. That goes through this chain:

This is for a static cache lookup, it might not add up to much with a regular cache, as the actual cache lookup is then likely much more expensive, but for a static cache that's expected to be near-instant, this adds up. About 50% of the cost is in optimizeTokens().

Steps to reproduce

Proposed resolution

I'm not sure, just some ideas:

* For the specific case of only having a single cache context (I'm profiling with anonymous user, it's user.roles), running optimizeTokens() seems pointless. It will never be able to optimize a single context away, so we could add an early return?
* A static cache in convertTokensToKeys? but we'd need to understand if and when context values can change during a request.
* Using VariationCache as a static cache like that is obviously fairly expensive, I can definitely see an argument that maybe flexible_permissions/group shouldn't be using it like that or should have some higher-level caching or other logic changes. group specifically always collects 3 different kinds of things in \Drupal\group\Access\GroupPermissionChecker::hasPermissionInGroup() but will always only use 1-2 of those and never all 3.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Active

Version

11.0 🔥

Component
Cache 

Last updated 1 day ago

Created by

🇨🇭Switzerland Berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @Berdir
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    So couple things for other people reading this:

    • Cache contexts are supposed to be fast, so that we can load variable things without having to run too much code to figure out the cache ID in VariationCache::createCacheIdFast()
    • If your cache context needs to load something (e.g. user roles), that needs to be cached (e.g. entity cache) so your next return value can be calculated quicker
    • Trying to make convertTokenToKeys run fewer requests to the cache contexts is, as pointed out in the IS, dangerous territory as the return value may have changed over the course of the request

    With that out of the way, I need to point out that this is no longer a Group-specific thing, but a core thing. We now have Access Policy API and, just like Group, it uses a persistent and memory variation cache to store your calculated permissions. So if your permissions are highly variable, we have a bunch of cache contexts to check whenever a permission check is ran. As far as that goes, I would definitely focus on making sure cache contexts are fast.

    For the specific case of only having a single cache context (I'm profiling with anonymous user, it's user.roles), running optimizeTokens() seems pointless. It will never be able to optimize a single context away, so we could add an early return?

    Agreed, we could make optimizeTokens return early if the count of the array is smaller than 2.

    A static cache in convertTokensToKeys? but we'd need to understand if and when context values can change during a request.

    Very dangerous territory as explained above.

    Using VariationCache as a static cache like that is obviously fairly expensive

    Yeah, but it's required for access policies to work. I think the cost-benefit is warranted here given how much is now possible with the access policy API.

    group specifically always collects 3 different kinds of things in \Drupal\group\Access\GroupPermissionChecker::hasPermissionInGroup() but will always only use 1-2 of those and never all 3.

    Yeah, fully agree we can optimize that in Group.

  • 🇳🇿New Zealand quietone New Zealand

    Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

Production build 0.71.5 2024