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

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.

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

Seeing similar results. Profiling data (Drupal 10.5.x, Group module enabled):

- convertTokensToKeys: 134 ms total, 1,104 calls
- createCacheIdFast: 92.4 ms, 1,032 calls
- optimizeTokens: 54.8 ms, 1,511 calls
- getService from optimizeTokens: 3,775 calls

Tested optimizations:
- isset() instead of in_array() - no real gain
- Check ancestors before getService() - ~2.8x speedup
- Cache getCacheMaxAge() results - ~10x speedup

Proposed changes
1. Early return for count($context_tokens) <= 1
2. Check ancestors first, call getService() only when ancestor is found
3. Cache getCacheMaxAge() in class property (safe - static metadata, not runtime value)

I've added a merge request.

Changes look good, but we cannot cache the outcome of a cache context. Right now, their value may change throughout a request. Until we decide whether we actually want that or not, we have to support that.

So +1 for the optimizations you made, but -1 for the static cache.

Need to fix PHP Unit tests

In my project's reality, it will be better to have per-account keying. The PR is updated.

True as that may be, we need to look at everyone's project when accepting code into Drupal core :)

I left a comment, as the new PR still has some potential for edge cases going off the rails.

Hm, spent more time to understand what's wrong with the CI, I've created a new MR https://git.drupalcode.org/project/drupal/-/merge_requests/14424

Seems the CI failures are unrelated to infrastructure issues.

Appear to have 2 MRs now, 1 should be closed please.

Also proposed solution may need to be flushed out.

Sure, I've closed the old one MR, we need to check an existing opened MR

Tests are passing now, nice.

Left some minor comments.

Is it possible to add a test case for this? We want to ensure that the container is not called too often, we could test that with a mock prophet. For example that the container is only called X times when certain ancestors are present in the context tokens.

Starting to look a lot better, thanks :)

Added call count expectations to the existing test coverage, also simplified the mock container.

Sorry this one appears to need a rebase.

Conflicted with #3579897: Convert expectation-less test mocks to stubs - Cache library, rebased.

For the same reasons as in https://www.drupal.org/project/drupal/issues/3539161#comment-16391346 I don't think we need to avoid static caching here in case permissions change during the request etc. as long as we always check the current user, if you have access to something at the beginning of the request you should also have it at the end of that request otherwise pages etc. could be broken.

If we eventually add frankenphp worker support we'll need to segment the cache by request but that will be the case for lots of things.

I wasn't aware of this issue, but '’ve been working on the same problem in #3582977: Add static caching to convertTokensToKeys(). I still need to compare the approaches. If it makes sense, I'm happy to bring over any useful parts of my work to this issue.

Looked into this some and both approaches seem complementary:

1. This issue reduces the cost of each optimizeTokens() call by checking ancestors before calling getService(). This helps both anonymous users and authenticated users.
2. My approach in #3582977: Add static caching to convertTokensToKeys() takes a different approach. On a typical Umami page, convertTokensToKeys() is called 800+ times, but only ~30 of those are unique token sets. My cache is for anonymous users only, and eliminates ~97% of those calls entirely.

I merged our approaches into one and I did some rough, unscientific benchmarking from the airport this morning: your optimizeTokens() improvements give 6-8% for both anonymous users and authenticated users. The static cache adds another 4-5% on top for anonymous users. Combined: 11-13%. The combined cache also lowers CacheGetCount in StandardPerformanceTest further.

@kristiaanvandeneynde and others: you repeatedly mentioned that static caching in convertTokensToKeys() is dangerous because values can change mid-request. I learned that the hard way in #3582977. :-) However, for anonymous users, I’m not sure how cache context values can change mid-request (outside of subrequest boundaries, which we handle). If they could, the render cache and dynamic page cache might be broken for anonymous users? Maybe that assumption is wrong though ... If so, I’m curious if we could create a test for that because all tests pass in the other issue.

I didn't want to commit directly to your branch, but here is a patch with the static cache layered on top of your work. It should apply cleanly against your latest code.

The static caching may be saving on some of the same calls that #3539161: Static cache access policy checking is trying to drop (originally via a static cache but we thought of an approach on there that might not need one).

I started looking at #3539161: Static cache access policy checking again, then I realised we probably need to resolve this first, it fixes some things that make that one harder to do, or might make it unnecessary altogether.

Rebased and tried to address @klausi's review comments.

Needed to update StandardPerformanceTest a bit more - the same three cache gets we save in one assertions are saved in a couple of others too. Tests are green again now.

Nice, looks good to me! I like it that we are now testing the number of container service calls.

The reduced cache get counts in the performance test also prove that we are improving something here, really cool that we have those!

I think I am still eligible to commit this as I only made small changes to the test. I am also really excited to see this land as it's a significant improvement when looking at xhprof.

Committed and pushed 8bf67fbd480 to main. Thanks!

Let's also backport this to 11.x, doesn't apply there because of the performance figures.

Cherry-picked to a new 11.x branch, only got merge conflicts on the unit test changes. Had to backport more unit test changes to make that work - ended up checking out the entire file rather than trying to locate individual commits.

Thanks @catch! You're right that the user can change from anonymous to authenticated mid-request. But I think it is safe because on every call to convertTokensToKeys(), we check whether the current user is anonymous before looking at the static cache. Once the user becomes authenticated (whether via AccountSwitcher, login form, or magic link) that check fails, and we skip the cache entirely. But maybe you mean something else?

I've also been thinking: is there a test I could write that invalidates this idea? If so, it would be good to add to Core (instead of this change).

Committed and pushed 160ab21c68d to 11.x. Thanks!

Let's continue with the static cache optimizations in the other issue.

@Dries did you mean to post your comment on #3582977: Add static caching to convertTokensToKeys() ?

@catch: Yes, sorry about that! Too many tabs open.