Review cache bin and cache tags of access policy caching

Created on 18 January 2025, 3 days ago

Problem/Motivation

This is about two caching things in AccessPolicyProcessor.

See #3436146-9: Introduce a list of "common cache tags" to reduce lookup query amount . The access policy includes dynamic cache tags for user roles very early in bootstrap.

Those can't be easily preloaded and preloading them all requires more complex preloading logic.

The question is whether it's worth to add cache tags for specific roles there. Roles typically don't change often, it might be worth to just use the list cache tag. This is relatively minor and might actually mostly go away with the ideas below for just core at least.

The second part is that it uses a separate, new cache bin just for itself. For the database backend, this doesn't make a big difference.

But for redis/memcache, each unique bin is a performance cost, because redis/memcache have no means to delete/invalidate across a whole bin. Redis maintains both a separate last-delete-all timestamp per bin and a cache tag for invalidateAll (See 📌 Deprecate CacheBackendInterface::invalidateAll() Active and 🐛 Allow to remove support for invalidateAll() and treat it as deleteAll() Active .

Right now, the current access policy cache implementation therefore results in 5 operations based on the monitor output from 📌 Optimize bin cache tags and last write timetamp Active :

"HGETALL" "core:access_policy:access_policies:drupal:[user.is_super_user]=1:[user.roles]=authenticated,administrator"
"GET" "core:cachetags:x-redis-bin:access_policy"
"GET" "core:access_policy:_redis_last_delete_all"
"HGETALL" "core:access_policy:access_policies:drupal:[languages:language_interface]=en:[user.is_super_user]=1:[user.roles]=authenticated,administrator"
"MGET" "core:cachetags:config:user.role.authenticated" "core:cachetags:config:user.role.administrator" "core:cachetags:access_policies"

Explanation:
1. first cache get, the redirect
2. this needs to check if there was an invalidateAll() on the bin, so it checks the cache tag for it. This could be skipped with the referenced invalidateAll() issues.
3. then it needs to check if there was a deleteAll() on the bin
4. then the real cache get with the redirected cache key
5. then it checks the 3 specific cache tags in my case for admin.

All of this is purely done to support more complex implementations that the majority of sites are actually not using. For the implementation in core, the only thing this is caching is the roles, which are in the ChainedFast config bin.

This is a lot of lookups for what it's doing, so setting to major.

Steps to reproduce

Proposed resolution

I'm not sure. For core, it could use discovery bin, but I understand that sites using group module for example might have a lot of different cache variations. But we IMHO need a way to avoid all this overhead for the vast majority of sites that don't have expensive access policies.

What if we add some kind of interface or method to indicate whether access policies are actually worth to cache? Group can do that, Core won't, and then we just use the static cache if none of the access policy services we have require persistent cache?

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.1 🔥

Component

user 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
  • 🇨🇭Switzerland berdir Switzerland
  • 🇬🇧United Kingdom catch

    The interface seems worth a try. In that case would we keep the separate bin for when it's used?

  • Pipeline finished with Failed
    3 days ago
    Total: 550s
    #399997
  • 🇨🇭Switzerland berdir Switzerland

    Created a merge request, works as expected, combined with the improvements in core (such as the theme extension list cache) and redis (the already committed last write timestamp and the invalidateAll() optimization), this is what's left now up until dynamic page cache

    "HGETALL" "core:container:service_container:prod:11.2-dev::Linux:a:4:{i:0;s:4
    :42:\"modules/contrib/redis/example.services.yml\";i:3;s:40:\"modules/contrib
    "GET" "core:container:_redis_last_delete_all"
    "GET" "core:last_write_timestamp_cache_config"
    "GET" "core:last_write_timestamp_cache_discovery"
    "GET" "core:cachetags:entity_types"
    "HGETALL" "core:data:route:[language]=en:[query_parameters]=:/"
    "GET" "core:cachetags:route_match"
    "GET" "core:data:_redis_last_delete_all"
    "GET" "core:last_write_timestamp_cache_bootstrap"
    "HGETALL" "core:dynamic_page_cache:response:[request_format]=html:[route]=vie
    
    "GET" "core:dynamic_page_cache:_redis_last_delete_all"
    "HGETALL" "core:dynamic_page_cache:response:[cookies:big_pipe_nojs]=:[languag
    

    (Thee's quite a bit more after, such as placeholdered blocks, library and assets, routes being loaded).

    This is getting pretty close to what's feasible. The only additional thing I can think of right now is the cache tag preloader, I'm seeing the following standalone and stable cache tags:

    "GET" "core:cachetags:entity_types"
    "GET" "core:cachetags:route_match"
    "GET" "core:cachetags:library_info"
     "GET" "core:cachetags:routes"
    "MGET" "core:cachetags:config:shortcut.set.default" "core:cachetags:config:user.role.authenticated" "core:cachetags:config:user.role.administrator" "core:cachetags:access_policies"
    

    4 easy ones, to reduce 4 get requests to a single mget.

    The last is shortcut and interesting, that one still contains the access policy cache tags. they are still collected and I guess shortcut adds that as a dependency to it. but there's also the shortcut set itself. for access policies, I still feel like role save should just directly invalidate access_policies, and shortcut could maybe use a single list cache tag. hard to predict if there is a scenario where shortcuts frequently change? Then we could consider to preload that too, but that's about saving a single extra query and likely only in some cases.

    There is of course also the huge list from dynamic page cache itself, but as argued there, it's impossible to preload that whole, so not much point in looking at that in this scope.

  • Pipeline finished with Success
    3 days ago
    Total: 476s
    #400010
  • 🇨🇭Switzerland berdir Switzerland

    Performance tests are very unreliable for me locally. script size is like 4kb off and everything goes completely haywire if I don't add the explicit cron run in setUp(). Is my system too fast? too slow? is it WSL? different chrome version? Honestly no idea.

    I was also confused why don't see a change on cache get requests with this on any performance tests except one single cache tag is valid count being reduced in a single test.

    The thing is that this trades regular cache requests with fast chained requests, but for Performance tests, those are the same thing. I don't think they should be. Obviously a separate issue, but what about grouping the cache operations per bin? We can keep the current methods and then aggregate, but we can also add methods to assert counts on specific bins and add those to select tests?

  • 🇬🇧United Kingdom catch

    Assertions bin sounds good and better than fast vs non fast - could you open an issue?

    Shortcut currently isn't being used in Drupal CMS (since you can do most of what it's used for with menu UI + navigation anyway) so could be a candidate to remove from core.

    Just directly invalidating access policies on role save seems reasonable but I think we need Kristiaan's input on that.

  • 🇬🇧United Kingdom catch

    Performance test/cron issue is https://www.drupal.org/project/drupal/issues/3500489 🐛 Performance tests need to run cron Active and needs review.

  • 🇨🇭Switzerland berdir Switzerland

    Created 📌 Aggregate cache operations per bin to allow for more specific asserts Active for the cache operations by bin stuff.

  • 🇨🇭Switzerland berdir Switzerland

    > In that case would we keep the separate bin for when it's used?

    Good question, I'm not sure. Really depends on the implementations of the used access policies. I imagine that with group or so, there could be a fairly large amount of variations, but they should be quite small?

    Sites that want to optimize this can configure the access_policy bin use ChainedFast, it's still a separate bin though. Updating it to use a different/existing bin is a little bit weird, since the service name and properties are all directly tied to that name.

  • The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • First commit to issue fork.
Production build 0.71.5 2024