- Issue created by @berdir
- 🇬🇧United Kingdom catch
The interface seems worth a try. In that case would we keep the separate bin for when it's used?
- Merge request !10957Allow access policies to flag that caching is not required → (Open) created by berdir
- 🇨🇭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.
- 🇨🇭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.