Use cache collector for state

Sorry for the noise, another conflict on 10.0.

And a reroll for 10.1, same conflict for 10.0 and 10.1, but the patches are elswhere not the same. 🐛 Cached services can't be used in service providers/modifiers Fixed is close now, once that's in I will check if there are any test fails left.

The last submitted patch, 117: 2575105-117.patch, failed testing. View results →

version of the patch from #106 without the tests, to have a patch that applies on D9. test changes are excluded to avoid rerolls for D9

sorry about the empty patch, here is again patch 106 with no test

Another reroll. There's no point in updating the deprecation message versions until the blocker is resolved.

The last submitted patch, 121: 2575105-121.patch, failed testing. View results →

Another reroll. 10.1 is busy.

The last submitted patch, 123: 2575105-123.patch, failed testing. View results →

Note for people following along here. 🐛 Cached services can't be used in service providers/modifiers Fixed is now a hard requirement to use this on 10.1 due to the new DevelopmentCompilerPass that use state. I'm not sure if state is the correct thing to do use there but for now, you need the current patch from that other issue to be able to use this.

Here's a combined patch with the one from the issue. I'm seeing some very weird things on 10.1 with Symfony 6.3 with this (something about Ghost/Lazy services and missing properties). Lets see what testbot thinks.

Can reproduce the errors also with just core on update.php. It's going through DevelopmentSettingsPass as expected, that's not visible in the backtrace though. Error looks like this:

Error: Call to a member function insert() on null in Drupal\Core\Lock\DatabaseLockBackend->acquire() (line 71 of core/lib/Drupal/Core/Lock/DatabaseLockBackend.php).

Somehow it doesn't work to properly initialize a Ghost object so early and then it explodes.

Workaround is to not use the state service there but use the key value store directly and bypass caching and CacheCollector stuff. Not pretty but avoids the issue and actually kind of makes sense as we are not interested in persisting those values in the cache.

The last submitted patch, 127: 2575105-2363351-combined-123.patch, failed testing. View results →

The last submitted patch, 128: drupal-core-init-cache_default_bin_backends-2363351-88.patch, failed testing. View results →

Ah, much better. Fixing that unit test, then this should green, but still blocked on the cached services issue, even the workaround for DevelopmentSettingsPass as there are some tests that break.

Based on your last comment this is postponed 🐛 Cached services can't be used in service providers/modifiers Fixed

That is in, rerolled. Conflicted on the deprecated state stuff.

Deprecation messages need updating. Same for CR please

All I did was to update two trigger_error messages.

I'm slightly torn on this. I strongly believe that this has considerable performance improvements, we're seeing large reductions of query counts with this on production sites.

That said, there are definitely cases where cache is _not_ required and it's wasteful to load things on every request that are only needed on cron jobs. I'm wondering if we should instead introduce a new service that wraps state and make caching an opt-in. It would implement the same service, so it's a drop-in replacement. Not sure about \Drupal::state(), could be a new method as we want to promote it IMHO, or a new parameter.

That would deal with concerns about sites/custom code that stores a lot of data in state that is not frequently. Thoughts?

Should we be moving some keys out of state to a separate collection? I was thinking about the recent development settings page since that's only accessed on container rebuilds. Not sure what that collection would be called though, like state-2...

We're comparing this to Drupal 7 variables which loaded the entire table of state + config on every request so we're already quite a bit more efficient, but yeah it does seems a waste.

So this issue doesn't stall should this be NW for #137?

@smustgrave if we did #136 it would be needs work for that, but if we do #137 it would not, instead we'd open a spin-off and go ahead here.

To keep the issue from stalling going to mark it then. If we need a follow up for #136 can do that for everyone here.

The last submitted patch, 135: 2575105-133.patch, failed testing. View results →

I am reviewing issues that I have bookmarked that were committed and then reverted.

As I read #136 and #137 there is not yet agreement on the approach so setting this to needs review for a decision on that point.

There are failing tests and this should be converted to an MR. I will do that now.

There was a conflict when applying the patch so here is the interdiff.

I am also tagging no-needs-review-bot to emphasize that a decision should be made before working on the MR.

Question who can make the decision though?

@berdir or @catch could you make the calls the remaining tasks? Not sure who can.

@berdir's idea is to opt-in individual keys for caching.

My idea is two have two services/collections - one for cached state things, one for infrequent.

I think two collection is more robust - no chance of something being requested both cached and uncached, but we need to name it, and we also need to decide whether the new or old collection is the one that gets the caching.

Once possibility would be state vs. state_cached (or state.cached) and move the frequent stuff over to it.

I don't think I was thinking of doing it opt-in per key, I also assumed we have two state services, not two collections though. Although I suppose that does kind of make it per-key.

I'm not sure I get how exactly we would "move" stuff over. Wouldn't that essentially be an API change? Lets take the maintenance check for example. If we just switch core over, then other modules and for example drush that check the old one will be broken as that will just not be there anymore. We'd need a BC layer on the non-cached state (which I assume would be the current state collection) that redirects keys to the cached version, so we need to somehow give it a list of all those keys, which sounds complicated.

The thing is, we already have non-cached collections, that's every other collection stored in keyvalue storage. If you don't want to have it cached, use \Drupal::keyValue('yourmodule')->get('foo'), that's just a few extra characters (DI is a bit more involved with the factory). The only reason we'd bother with a cached and non-cached state version is it wasn't cached until now and there might be sites out there that have all kinds of things in there due to custom and maybe some contrib modules using it ways it's not really meant to be used.

Of course "the way it's meant to be used" is kind of being changed here, no existing documentation clearly says it should only be used for small bits of information and limited cases, it's just somewhat implied by the examples. Also, the fact that it uses the key value store and is "just" a specific key value collection and the key value API itself doesn't seem to be actually documented anywhere (except the query to get all keys on the documentation page). Neither on https://api.drupal.org/api/drupal/core%21core.api.php/group/info_types/10 nor is there a key value API on https://www.drupal.org/docs/develop/drupal-apis → , and https://www.drupal.org/docs/8/api/state-api/overview → (which I think is very confusing, especially the reset part).

Anyway, a counter-proposal/idea to the two different things idea:
* We introduce a setting that in D10 defaults to FALSE, so you have to opt-in to get the cache, with a requirements check that points out if the setting is not explicitly set and recommends to either set it to TRUE or FALSE explicitly
* in D11+, we switch the default to TRUE.
* We improve the state documentation to make clear in which cases it should be used and when it's better to use your own key value collection
* Consider moving some examples in core to a different key value collection to have fewer bad examples, coming back to the BC concerns though. I don't think we need to provide BC for example for the twig development mode setting though?

(Also setting to needs work so that @smustgrave no longer needs to be sad about this being stuck in the needs review queue ;))

+++ b/core/lib/Drupal/Core/Routing/RoutePreloader.php
@@ -73,17 +68,7 @@ public function onRequest(KernelEvent $event) {
 
-      // Ensure that the state query is cached to skip the database query, if
-      // possible.
-      $key = 'routing.non_admin_routes';
-      if ($cache = $this->cache->get($key)) {
-        $routes = $cache->data;
-      }
-      else {
-        $routes = $this->state->get($key, []);
-        $this->cache->set($key, $routes, Cache::PERMANENT, ['routes']);
-      }
-
+      $routes = $this->state->get('routing.non_admin_routes', []);
       if ($routes) {

if we add the setting, what are we going to do with this bit? drop the cache, sites with disabled cache will just get an extra query? Add extra complexity and do a check for the cache setting, only doing it if global state cache is disabled?

Consider moving some examples in core to a different key value collection to have fewer bad examples, coming back to the BC concerns though. I don't think we need to provide BC for example for the twig development mode setting though?

This sounds good and I also don't think we need to provide BC for that one (hopefully none of the ones we move, but definitely not that one).

The $settings opt-in also sounds like a good plan.

#149 I could go for either of those approaches, not sure which is best, but at least it'll be temporary.

It's a bit ugly, but I implemented the setting now in State, have to override a bunch of methods to avoid unnecessary cache get/set/invalidate calls.

This should have an effect on the new performance tests in core, haven't looked much at them yet, I guess that's going to change now.

From https://git.drupalcode.org/project/drupal/-/merge_requests/5636/pipeline... looks like we're saving approx 10 database queries on nearly every page request, which is great and what I'd expect based on how many state checks we do usually.

Also there's a possibility it's a fluke but the full test run was 7m53 seconds with all functional tests finishing under 4 minutes - it may be enough of a change that it's making test runs faster overall.

Consider moving some examples in core to a different key value collection to have fewer bad examples, coming back to the BC concerns though. I don't think we need to provide BC for example for the twig development mode setting though?

Yeah I think that's fine - we can add a change record and the specifics of whether and when keys are set aren't part of the public API, obviously if contrib is actually relying on something a lot we'd need to think about it more carefully, but we can do that case-by-case.

> From https://git.drupalcode.org/project/drupal/-/merge_requests/5636/pipeline... looks like we're saving approx 10 database queries on nearly every page request, which is great and what I'd expect based on how many state checks we do usually.

Yes, saw that too. I was confused about the increase of queries in that umami test though, I need to look at what that exactly does, I wonder if there's some timing issues/state cache race conditions as it does multiple requests in quick succession.

Reproducing that manually in blackfire with a user without any permissions in umami gives me _very_ different numbers and what I expect. -4 state queries, +1 cache_default query, 33/3 in total. And not 17/18.

I think now that it's opt in, it would make more sense to move this to cache_discovery so we use the fast-chained backend for it? That should save another query then.

Checking on two separate production databases that I work on:

> select name, length(value) from key_value where collection='state' and length(value) > 10000;
+--------------------------------------+---------------+
| name                                 | length(value) |
+--------------------------------------+---------------+
| drupal_css_cache_files               |         30824 |
| routing.non_admin_routes             |         16115 |
| simple_sitemap.queue_stashed_results |       8136706 |
| system.js_cache_files                |         70931 |
+--------------------------------------+---------------+
4 rows in set (0.005 sec)

> select name, length(value) from key_value where collection='state' and length(value) > 10000;
+--------------------------------------+---------------+
| name                                 | length(value) |
+--------------------------------------+---------------+
| drupal_css_cache_files               |         10519 |
| simple_sitemap.queue_stashed_results |       6331055 |
| system.js_cache_files                |         11710 |
+--------------------------------------+---------------+
3 rows in set (0.001 sec)

Looks like simple_sitemap should be storing whatever that data is somewhere else.

Reproducing that manually in blackfire with a user without any permissions in umami gives me _very_ different numbers and what I expect. -4 state queries, +1 cache_default query, 33/3 in total. And not 17/18.

One thing to note is that cache requests in the performance tests include those from chained fast, so if blackfire is ignoring those, it'll be a huge difference.

I think now that it's opt in, it would make more sense to move this to cache_discovery so we use the fast-chained backend for it

Seems sensible to me.

RefreshVariablesTrait calls $state->resetCache() which in the MR calls clear() which resets both the persistent and static caches, it needs to be ::reset() instead. Pushed a commit. Test should fail in the right direction now.

#156 makes the change in the performance test look right to me, however we're seeing other test failures in runs which aren't consistent. I think the issue is that any test using state to track.. well.. state between requests, now needs to either switch to WaitTerminateTestTrait or to use the key value service directly without the state wrapper - because the cache will be updated in terminate. Pushed a commit for LanguageNegotiationContentEntityTest which was failing locally for me.

Before I found this, I thought I found a race condition in the state implementation - the cache entries were being invalidated before key value is written to. That's not the cause of the test failures after all, but I still think it's a valid change.

Got to a green test run, but had to do some horrible things in LanguageNegotiationContentEntityTest and some of the test failures are random so could still be a couple of false positives.

I think the proper approach to those is going to be switching from state to raw key value in the test modules/test classes, if we do that we can probably undo TerminateWaitTestTrait. However this at least confirms that all of the test failures are due to test and tested site going out of sync when explicitly using the state API to track/alter things in the test/test modules themselves, and not real bugs caused by the change at all.

Any recommendation for reviewing this one?

I think the main remaining things are:

1. We should open issues for taking things out of state and into key/value directly which are accessed extremely rarely. The new development settings stuff that's only on container rebuild / form save are the obvious ones, there are probably more though.

Also an issue against simple_sitemap would make sense too.

2. Decide whether the changes in core/modules/language/tests/src/Functional/LanguageNegotiationContentEntityTest.php are acceptable here with a follow-up to move the test modules to use key/value, or whether we should refactor those test modules to use key/value in this issue. This also goes for the various test modules used in the tests that are getting WaitTerminateTestTrait added. I could go either way here - refactoring the test modules adds scope, but it'll be less churn because we then wouldn't be adding WaitTerminateTestTrait and removing it again.

Tagging needs follow-up since we need that for #1 even if we need to make a decision on #2.

Did a rebase after the query logging changes. Ignored all changes to StandardPerformanceTest and OpenTelemetryAuthenticatedPerformanceTest, that will need to be redone completely.

Couldn't run either test on my current system, both fail with "Test was run in child process and ended unexpectedly", will need try somewhere else.

I'll take care of the performance stuff real quick

Went green on my local (the performance stuff)

Thanks, that looks great.

One thing I find a bit interesting is that the cache get count goes up. But we're also removing a cache get here in RoutePreloader, does that not run on these requests? Maybe because all these test scenarios actually high dynamic page cache.

Would be kind of nice to have a test that doesn't use that as that's hiding _a lot_ of stuff, but I guess it could also get a bit overwhelming, that's going to be a lot of queries I guess. and not in scope of this issue.

@berdir see 📌 Route preloading doesn't run on dynamic page cache hits Needs review .

There are some cold/lukewarm cache tests in the Umami performance tests which don't do query assertions yet, I've been hesitant to add assertions to them while we had so many random test failures, but we should probably try that now. This isn't the same as no dynamic page cache at all though so we still might want that too.

I would assume the extra get is coming from the collector going to the cache once to get all of the state stuff, whereas before the state stuff were non-cache-related queries, which showed up in the list of individual queries we're tracking.

Quick guess: The cache get that is saved in RoutePreloader shows up in those test cases where the cache get count does not go up by one. The ones where it does go up, RoutePreloader doesn't save a cache get and state introduces one extra get. Just a guess, though.

Edit: Cross-posted with @catch and my guess seems to match what he's saying.

Fixed tests after 📌 Replace REQUEST_TIME in services Needs work

Last fail is Drupal\Tests\media\FunctionalJavascript\MediaSourceOEmbedVideoTest::testMediaOEmbedVideoSource, I'm not getting any wiser from the artifacts. I'm not seeing anything related to media in the changes either.

Worth running the job again to see if it's an existing random. I don't remember that one specifically but there's been a lot of random media test failures.

The test reported already but only once in 168 and error is different, so maybe it's real issue with redirect?

Behat\Mink\Exception\ExpectationException: Current page is
    "/media/add/test_media_oembed_type", but "/admin/content/media" expected.

it's because of error The provided URL does not represent a valid oEmbed resource. for https://vimeo.com/7073899

traced down the test locally (reproducible) til \Drupal\media\OEmbed\ResourceFetcher::fetchResource()

cURL error 28: Operation timed out after 5003 milliseconds with 0 bytes received (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for http://172.30.1.2/media_test_oembed/resource?url=https://vimeo.com/7073899&altered=1

Why can't I do a fast-forward pull on this branch? Happened twice now already 🤔

Re #172, are we sending a GET request to Vimeo every time we run our tests? I'm not sure how happy Vimeo would be with us doing so :)

@kristiaanvandeneynde no, there's mapping in state to return local file and somehow this request is failed.

I used to add debug print and I see that request failed before the controller returns result(

Very interesting work. Xhprof lead me to this issue, as I am tracking down a performance issues in a D10.2.x —site.

select name, length(value) from key_value where collection='state' and length(value) > 10000;

 custom_fs_connector.basic_data          | 808712
 custom_fs_connector.info_type_name_list |  27342
 custom_room_info                            | 304317
 custom_fs_connector.course_code_list    | 101678

This is information that not often change, tough by reading through the issue and try to understand what is going on, I would say, this data should not be stored here but the site custom module. From my understanding is this data that not often change.

Ah yes, now that you mention it: ResourceController::setResourceUrl() uses state.

Then the failure makes slightly more sense, we're changing how state works and now a test using state is failing.

Rebased, I expect the performance tests to fail as I couldn't figure out what the new expected counts were supposed to be from the conflicts.

Also reverted the AutomatedCron event subscriber priority back to 100 and the corresponding change in cron.api.php, I think now that needs_destruction is handled after all terminate events (following 📌 Allow needs_destruction services to run on page cache hits Needs review ) that all terminate event subscribers are safe to use state.

@kristiaanvandeneynde fast forward pulls do not work when the branch has been rebased and force pushed, most MR contributors tend to use the rebase method instead of merging in 11.x as merge commits are sometimes tricky to work with and it makes the set of commits in the MR cleaner.

Maybe a can of worms but is this another use for a feature flag module over settings.php? We could enable the feature flag module on new installs but existing sites can opt in over time, and eventually we can deprecate/remove it?

MediaSourceOEmbedVideoTest sets state during the test and then expects the site under test to read that, but now we don't flush state until terminate so the test doesn't have the effect it thinks it does. To solve this I switched the test ResourceController to use key value instead of state.

Also updated the performance test counts.

ExtensionList::getPathNames() appears to wrap cache around state, we can perhaps drop the cache wrapper in a followup.

In general this looks really good! I'm wondering why we're adding the WaitTerminateTestTrait without calling its method, though? I couldn't find anything in core that automatically calls that method if it's present either.

Removed WaitTerminateTestTrait seemingly with no harm done.

Removed the cache wrapper from ExtensionList, hoping this will remove another cache get.

Just went over all the changes again and they look good to me. I would just like to see a follow-up so we don't forget to change the state_cache setting in D11.

Also wondering if we need a deprecation notice for dropping the $cache argument from RoutePreloader, although I couldn't find any other example of that in core or the documentation → . The docs describe what to do when A, B, C becomes A, C; but not what to do if it becomes A, B. I would err on the side of adding a trigger_error still, using func_get_args().

I have no other objections. Good job on figuring out MediaSourceOEmbedVideoTest, by the way! Setting to NW for the 2 points above but feel free to ping me when those are addressed and I'll gladly set this to RTBC.

Thanks for reviewing, opened 📌 Use cache collector for state Needs review .

#160 asked for some other followups, so leaving the tag in place for now.

Will add a deprecation for the RoutePreloader argument, and also change LanguageNegotiationContentEntityTest to use keyvalue in this issue, given we have precedent in the ResourceController for the OEmbed test.

Also opened 📌 simple_sitemap.queue_stashed_results should be in keyvalue instead of state Fixed , unsure if @catch wants to open any more followups or not - leaving the tag in place for now.

Added a release note given we are adding a new setting here.

The tests failed a couple of times but I think I was just unlucky with random fails - they just passed again so hopefully this is all good now.

Tagging as a release highlight, because along with other changes in Drupal 10.3 we're dramatically decreasing the database queries on all page requests, can go in a general 'performance improvements' paragraph.

I opened 📌 Move the TestWaitTerminate middleware to a module Active which I'd been thinking about for a while, but seeing the query in the diff reminded me of it.

Now that tests using state have been updated to k/v here, I think that's everything I was worried about in #160 so removing the tag.

RTBC for me, should fix the 2 drupal:10.0.0 references to drupal:10.3.0, though. Holding off on those grounds.

Fixed the deprecation version, don't know what I was thinking there.

Agreed. Code looks good to me now, so RTBC.

One more follow-up, I opened 📌 Move twig_debug and other development toggles into raw key/value Fixed .

Really nice to be able to commit this one, we've come a long way since variable_get().

I had to resolve a merge conflict on 10.3.x, it was trivial, let's see whether I did I right or not...

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

catch → closed merge request !5636

Wow. Very tempted to insert the "It's been 84 years..." meme. This took a while, thanks for the help to push this over the finish line.

I've clarified and expanded the change record: https://www.drupal.org/node/3177901 → , specifically around the current opt-in/opt-out behavior and how to check for problematic usages of the state API.

I've also added a short paragraph on recommended/correct usage of the state API and I created a follow-up issue to improve API docs as well as the documentation on drupal.org: 📌 Improve documentation of the state and key value API Active

This feels like a good heuristic:

Sites with more than 100 entries and/or a total value size/length of 100'000

Wonder if we should add a hook_requirements status check for this to help people before Drupal 11 enables this for good?

I just made those numbers up, felt like a nice, even numbers, not sure about them yet, especially the length one.

There is already a hook_requirements() warning, the problem is that there is no API to get that information, we would need to check the used key value implementation and then hardcode them or something like that, was not sure about the putting that complexity in there. What meant to do is link the change record there in that message so that people can read through that if they have questions. Maybe we can do a follow-up commit or issue for that?

Question: we have a sub-class of State in ECA, how should we declare the constructor and the service, if we want to support both Drupal 10.2 and 10.3? This seems impossible with this change, or am I missing something?

I found https://git.drupalcode.org/project/eca/-/commit/3bb22ea5820fd81f72e46c44...

Since this is a subclass with its own service definition, why pass the extra services into your own constructor for both 10.2 and 10.3 - they won't be used in 10.2 but that seems OK?

Unfortunately not. We have an extra service to inject and our service declaration looks like this:

  eca.state:
    class: Drupal\eca\EcaState
    parent: state
    arguments: ['@datetime.time']

So, for 10.3 the constructor needs to look like this:

public function __construct(KeyValueFactoryInterface $key_value_factory, CacheBackendInterface $cache, LockBackendInterface $lock, TimeInterface $time) {

but for 10.2 it needs to look like that:

public function __construct(KeyValueFactoryInterface $key_value_factory, TimeInterface $time) {

@jurgenhaas why can't you inject cache and lock into 10.2?

@catch ah, you mean I could update the service declaration there to mimic what 10.3 is going to do in the parent service? They should work, I'll give that a try in 📌 Add support for cache collector for state in core Active

Spotted two issues here that deserve followups:

1. The new constructor calls the CacheCollector constructor with a fixed cache key; this won't work in subclasses, so we should move this to an overridable class constant.

2. We inject the bootstrap cache bin in services.yml but the BC fallback uses the discovery bin.

#206.1 can actually just be solved by overwriting the property, but opened 📌 State cache collector uses a fixed cache and collection ID Active anyway.

#206.2 is a bug so opened 🐛 State cache bin is inconsistently chosen Active

This is leading to random test failures, I opened 📌 [random test failures] Use key/value directly instead of state in test modules Active , no MR yet.

Automatically closed - issue fixed for 2 weeks with no activity.

We have observed that on some of our websites

SELECT name, LENGTH(value) FROM key_value WHERE collection = 'state' ORDER BY LENGTH(value) DESC LIMIT 100;

yields mength numbers in orders of 200k, which is not ok according to https://www.drupal.org/node/3177901 → .

The vast majority of the length comes from drupal_js_cache_files.

https://www.drupal.org/node/3177901 → recommends optimizing if

Sites with more than 100 entries and/or a total value size/length of 100,000 should review their specific entries using the following query and either optimize their usage or disable state caching as a temporary measure:

but how can I optimize something that originates from the core?

Could you file a new issue about this please? If core is setting that much data through drupal_js_cache_files, that definitely needs a dedicated issue.

Core removed any usage of the string system.drupal_js_cache_files 12 years ago when it was renamed to system.js_cache_files. And that too was deprecated and removed in in 10.2 or 10.1.

Whatever you have there is probably not core or at least not actively used anymore. Try searching your code base for it, if you don't find anything, you can delete those records. FWIW, since this is a cache collector, if nobody *uses* those state keys, they will also not end up in the cache.

No occurrence of drupal_js_cache_files in the codebase, it seems this is stale stuff. The website we spotted this on used to run on all versions starting D7, so maybe it is an artifact from these times.