Add a file parsing cache collector to replace some uses of FileCache

Pipeline is green.

There is one remaining thing, which is we need a new FileCacheGarbageCollectionInterface, implement it in the new class, call the ::garbageCollection() method in system_cron() if the service implements the interface. That will complete the expires logic added so the database table doesn't have stale and un-removable cache entries in it forever.

Pushed a commit to implement #4.

It's my attempt at garbage collection breaking. Pushed a commit which fixes at least one test.

MR is green again now. Updated the issue summary with a summary of the implementation.

Tagging beta target because I think this is needed to mitigate the memory usage implications of the OOP hooks bc layer and new attribute parsing in core generally.

Addressed the last comment. Did some manual testing of this with #3478621: Add filecache to OOP hook attribute parsing and got what I would expect running the UI installer with the standard profile. Comments over there.

When doing that testing I realised another thing.

FileCache forces use of the apcu_prefix in the cache key. I think we should move that down to the apcu backend so the database doesn't use it - this would enabling persisting file_cache entries across deployments which is potentially a few hundred milliseconds downtime and dozens/hundreds of mb of peak memory usages removed from every deployment. However will start a new issue to explore that and we can either merge it back into here or do it in two issues depending on what it looks like.

Also notable is that I'm only seeing hook implementations go into the cache, nothing from YAML parsing - need to refresh my memory on what we do there but looks like we might need to either fix or add some caching to library/info/config YAML parsing.

I think we want to add the table to the schema only suggestion in \Drupal\Core\Command\DbDumpCommand::configure()

This is done.

we also need an issue against Drush to update the schema only defaults stuff there for db dumping.

Happy to open this one once we've finalised the table name.

Also notable is that I'm only seeing hook implementations go into the cache, nothing from YAML parsing - need to refresh my memory on what we do there but looks like we might need to either fix or add some caching to library/info/config YAML parsing.

This was bad testing on my part - I already had these cached in APCu from a previous installation attempt. Did a ddev restart, emptied the database, reinstalled, and there were 317 rows just from hitting the first page of the UI installer with a pre-filled settings.php.

After install and hitting the front page, there were 1226 rows from various discovery (library, info etc.) - this was without the hook discovery caching MR.

Shows that there are definitely advantages to the APCu cache for situations like repeatedly reinstalling Drupal as well as test pipelines.

Opened #3488925: Move the FileCache apcu prefix handling into the apcu implementation or allow the APCu prefix to be customised and in the process of writing the issue summary, decided it definitely needs to be its own issue if we do it at all.

Moving discussion from the MR to here.

@alexpott brought up the test expectation change in InstallerExistingSettingsTest.

We added that in #3103529: Drupal 8.8.1+ and 9 can fail to install in the web browser due to cache pollution to prevent cache pollution when hitting index.php then getting redirected to the installer.

I had a look at what changes would be necessary to prevent the table from being created in the early installer phase (when settings.php is pre-filled), and I wasn't able to find a good way to do this - because of the singleton approach in FileCacheFactory as opposed to it being a service (which is necessary for it being a dependency for container building), finding somewhere that's early enough and also being able to detect the installer state is not really doable. One possible option I didn't try yet (because it seems like a huge hack) would be checking for the install state globals in the implementation. Our entire 'early installer detection' relies on 'has a table in system_schema() been created yet' which itself is a massive hack that needs to be replaced too.

However, the bug we added that test for is for the regular cache backends which don't do mtime invalidation, so I think it's fine for this table to be created when settings.php is filled, the content of the cache doesn't vary by the install state or not, it just varies by the contents of the file - so why not rely on it in that very early install state.

Tests are back to green.

Fixed the one comment on the MR. Also realised we can/should take the database connection out of the constructor - theoretically at least this can be used without needing one, so might as well get the connection as late as possible.

Re-ran the three failing jobs - all green now.

I looked a bit more and I'm not really sure about @daffie's comment either.

With ::getConnectionInfo() we ensure the default database connection is configured, and then get a connection if it is.

With ::isActiveConnection() we check if there's an active connection (which will be the default database connection if it's active whenever this would be called), but then if it's not, we won't try to start a new connection.

To me ::getConnectionInfo() seems more robust, because it's OK if we start a new database connection here, we just need to know we have one to start.

In practice this might not actually make any difference to a site.

The fix that just got committed for #3486503: Add a file parsing cache collector to replace some uses of FileCache should mean we get a green run again, restoring the RTBC since no code changes here.

This test failure (not in the most recent run) is very strange, and i can reproduce it locally:

https://git.drupalcode.org/project/drupal/-/jobs/3513488


+····2·=>·Binary·String:·0x494e5345525420494e544f202264617461626173655f66696c655f636163686522202822636964222c2022657870697265222c202263726561746564222c202264617461222c202273657269616c697a656422292056414c55455320282264727570616c2e66696c655f63616368652e31312e322d6465762e2e6c3833444a62637057506668645372695a775a657268432d79736b54374139566b72624c736b705053494d3a6174747269627574655f646973636f766572793a44727570616c5f76696577735f4174747269627574655f56696577734163636573733a4e58687363526530343430504670493564537a6e4556676d61754c32354b6f6a443775346539615a774f4d3a44727570616c5f76696577735f416e6e6f746174696f6e5f56696577734163636573733a2f6275696c325836714e3969314c3759675149686247414776365f314e354f6f656468746264586237544e4d54623973222c20313734303530393230332c20313733323733333230332e3638332c2022613a333a7b733a353a226d74696d65223b693a313733323733333136333b733a383a2266696c6570617468223b733a35303a22636f72652f6d6f64756c65732f757365722f7372632f506c7567696e2f76696577732f6163636573732f526f6c652e706870223b733a343a2264617461223b613a323a7b733a323a226964223b733a343a22726f6c65223b733a373a22636f6e74656e74223b733a3534333a22613a393a7b733a353a22636c617373223b733a33363a2244727570616c5c757365725c506c7567696e5c76696577735c6163636573735c526f6c65223b733a383a2270726f7669646572223b733a343a2275736572223b733a323a226964223b733a343a22726f6c65223b733a353a227469746c65223b4f3a34383a2244727570616c5c436f72655c537472696e675472616e736c6174696f6e5c5472616e736c617461626c654d61726b7570223a333a7b733a393a22002a00737472696e67223b733a343a22526f6c65223b733a31323a22002a00617267756d656e7473223b613a303a7b7d733a31303a22002a006f7074696f6e73223b613a303a7b7d7d733a31313a2273686f72745f7469746c65223b4e3b733a343a2268656c70223b4f3a34383a2244727570616c5c436f72655c537472696e675472616e736c6174696f6e5c5472616e736c617461626c654d61726b7570223a333a7b733a393a22002a00737472696e67223b733a36343a224163636573732077696c6c206265206772616e74656420746f207573657273207769746820616e79206f66207468652073706563696669656420726f6c65732e223b733a31323a22002a00617267756d656e7473223b613a303a7b7d733a31303a22002a006f7074696f6e73223b613a303a7b7d7d733a31333a22646973706c61795f7479706573223b4e3b733a343a2262617365223b613a303a7b7d733a353a226e6f5f7569223b623a303b7d223b7d7d222c203129204f4e204455504c4943415445204b455920555044415445202263696422203d2056414c554553282263696422292c202265787069726522203d2056414c554553282265787069726522292c20226372656174656422203d2056414c55455328226372656174656422292c20226461746122203d2056414c55455328226461746122292c202273657269616c697a656422203d2056414c554553282273657269616c697a65642229,

There are multiple queries that just say 'Binary String' with the same long string. I don't know if they are errors, or informational, but they are weird and being picked up by the performance test query logger (apparently only sometimes).

Realised thinking about that - we can't log these queries in performance tests as database queries, because an item may or may not exist in APCu depending on whether it gets evicted or is ever set etc. So we would need to ignore them. We could also ignore 'Binary string' queries but I'd like to know what those are ideally.

Theoretically it might be possible to add a file cache listener to log get/store separately (like we do for caches), but that's going to require overriding the backend with a custom one and can't just decorate a service.

Discussed this issue a fair bit with @berdir in slack, and he suggested possibly only opting into the database-backed cache for attribute parsing, since YAML parsing may be faster without it (and shouldn't result in unfreeable memory usage). Still need to think more about that, but this is blocked on excluding anything from FileCache in performance tests at least.

So from that slack thread, about 800 database queries were due to #3490347: Replace ExtensionParser::scanDirectory() FileCache usage with InfoParser, we should postpone this on that issue.

The weird binary string 'queries' are also a blocker here I think at least until we know what they are, and so is excluding queries from the backend from performance tests since we can't guarantee whether anything comes from apcu or the db by design, so it'll need some work once that issue is done. And maybe being more selective about which file caches we apply it to (can be done with $default_config param to FileCacheFactory).

We could explore a performance logging decorator in performance_test module in a follow-up, probably could only catch things outside container building but that is fine for most of our use-cases, so even if we exclude this from performance tests which can later bring it back.

Berdir is right that the drush installer currently wouldn't benefit from this, however I would expect e.g. a drush cr after install to benefit because the entries will be cached by then (from the installer or previous drush crs).

However also from the pure memory reduction standpoint, I was seeing preprocess hooks in system module discovered as hooks and added to the container, which is a side-effect of not having #3490355: Add procedural hook short circuit per module or file.

So maybe the order should be:
#3478621: Add filecache to OOP hook attribute parsing - already in as of yesterday.

#3490355: Add procedural hook short circuit per module or file - try to reduce the amount of parsing we do.

I also noticed when checking what gets cached vs not, and it's visible in the performance test results too, that we are doing a lot of file scanning and parsing for plugins that aren't plugins.

e.g.

core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ScaleAndCrop.php
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/GDImageToolkitOperationBase.php
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Crop.php
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Resize.php
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Rotate.php
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Convert.php
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Scale.php
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Desaturate.php
core/modules/system/src/Plugin/ImageToolkit/Operation/gd/CreateNew.php

Only GDToolkit is the actual plugin, all the operations are not, so that is nine 'NULL' cache entries where there should be one.

If I run:

SELECT cid, data FROM database_file_cache WHERE data LIKE '%"data";a:1:{i:0;N;}%';

(which is looking for a filecache data NULL entry - what we use in plugin attribute parsing)
I get 139 results.

We could have an issue which adds an exception when a non-plugin is found in a plugin directory, and gradually move these out. This would save directory traversal, memory, and space in APCu. And then if/when we return here, that's 139 less database queries too. Will open a meta and a sub-issue for the GD toolkit to start.

An issue against drush to use its own batch system in drush si also sounds like a good idea, I'm a bit surprised it's not already doing that. However thinking about that, we might want to do that in co-ordination with #3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller, that issue also results in a lot less reflection/YAML parsing, although eventually the same number of files will be parsed, just not as many times. But installing x number of modules in drush, hitting one that needs a container build -> new process to continue, this would be a good balance if it's doable.

Actually another thought which would be a change here but not postponed on the info parser issue.

There are two kinds of things we're using file cache for - extension, services, hook parsing which are all done during container build and therefore need to use the singleton.

However config parsing, plugin discovery, route attributes when we have them, libary parsing - these are all from services, so could depend on a service.

So... I think we could change approach here and do the following:

- add a new file_cache.persistent service, and inject this into the various places we want to use it.

- this service can be linked to a file_cache.persistent_cache_backend service which uses an actual cache backend, we'll need a no-op tag invalidator to pass in is the only additional thing.

- this can use BackendChain (not chained fast because we don't need the invalidation support) with the APCu and database backends.

- that will in turn make it easier to swap out the database backend with redis or memcache

- cache backends support both multiple get and multiple set. Because this is a new service with a new interface, we can copy those aspects of the cache backend interface (+ mtime invalidation support) and that will make it easier to do a query-per-directory instead of query-per-file and similar.

- for YAML parsing we'd need to have a non-filecache option for YAML parsing then, don't want to cache things twice..

This would not help us with hook and service discovery then, but since route and plugin attributes also load PHP classes, and there'll be as many of those potentially, we can save the memory somewhere both on web requests and cli that way.

@alexpott yeah I think this is more interesting for plugin discovery to start with.

Config - it's not read often, but also you can end up with 1000+ config files in a sync directory and maybe 30 changing when you do a sync, so a persistent cache with a 3 month TTL and no invalidation apart from mtime might still help - but probably need to check how the db cache would actually compare vs. Symfony YAML parsing.

I guess if it's not a service, could just do the implementation that's already here for that (but maybe not default if we want to avoid it for container builds).

Thought about this a bit more and might have a plan.

We can't do the original idea here because service and hook discovery (and potentially other discovery) can happen when there's no database available. Also if we use the same model as FileCache we'd end up with a lot of database queries (both gets and sets).

I think instead we can do something like the following:

1. Add a file_parsing.cache database 'backend' which is not tagged as a cache backend. Just a new service using an existing backend that won't get cleared on drush cr etc.

2. Add a CacheCollector subclass where the cache item is an array of mtime and data and the key is file paths. This CacheCollector should handle the mtime validation etc. It should probably also handle some garbage collection (e.g. on set, remove items from the cache if the mtime is more than a year old and they weren't requested in the request).

3. Not entirely sure about this, but to conserve memory, we might be able to directly instantiate the CacheCollector in the discovery code paths rather than using a service, this is done for the runtime theme registry already. That way when discovery is complete, we can free up the memory. If that doesn't work, then services will work.

4. We can potentially convert one usage of FileCache to use this, and/or one thing candidate that doesn't use it yet - js minification is one I'd like to use it for. Once we've got something working it'll be possible to profile the difference.

We won't be able to use this for service or hook discovery, or at least not without a lot of changes, but we should be able to use it for libraries, js/css minification, route discovery (YAML and attributes), plugin discovery (YAML and attributes) at least.

By using a cache collector, we only have one database get and one set per discovery type, so a 100% cold cache (usually the installer, or the first time this code runs on existing sites) shouldn't have too much overhead.

Where we can use this for existing FileCache implementation, it will allow us to remove some large and low hit-rate items from APCu, as well as increasing the hit rate against the new backend.

Going to move this back to active since the original MR was more or less a dead-end, don't think there's any actual code to re-use from it.

OK made a start on a new approach.

New 'cache bin' which is normal except for not being tagged as a cache bin, this means it won't get cleared on drush cr etc. which is the whole point here.

New YamlCacheCollector class - this handles Yaml::decode(file_get_contents()) and mtime validation. Used it in library discovery to start with. Net reduction of code in library discovery that currently handles FileCache itself.

Couple of considerations:

- this increases the peak memory of LibraryDiscoveryParser because it currently doesn't have a static cache in there. I think we will save peak memory if/when we can apply this to plugin attribute parsing and it's cleaner to have it as a service. One way to reduce the peak memory would be to manually instantiate the cache collector inline in LibraryDiscoveryParser but that could mean more cache backend access/locking if it's used multiple times per request etc. Leaving it as a service for now.

- right now there is zero garbage collection at all, but we need something. I think a good start would be to override ::updateCache(), loop over all entries (that aren't getting set), and if anything is more than 2 years old, remove it, then call the parent - this will mean stale files that are never coming back get dropped eventually. If a system started using this and then stopped again, you could also get stale info that way although technically they could delete their own cache item in a post update.

To use this for plugin attributes, js and css minification etc. we'll need separate cache collectors. I think we can make a base class with an abstract ::parseFile() method which the YAML and other ones then inherit from.

Next steps:

1. Try to use this in routing. Router rebuild in Umami should show a bigger change in numbers than standard profile library discovery.

2. Add an implementation for js minification via Peast and use this to validate the base class idea.

3. Possibly try to apply to attribute parsing but I have a feeling that will be complicated by the plugin system, so maybe a follow-up if it's complext.

4. Garbage collection per notes above.

Adding route discovery support. Because routing uses YamlDiscovery, this means adding YamlDiscovery support - which means we'll be able to use that for YAML plugins too. Because YamlDiscovery isn't a service, added a subclass that accepts a YamlCacheCollector - this works and also saves memory. Did it without dependency injection to start with to get some numbers.

Before/after screenshots. Note these aren't like for like because the before includes everything that YamlDiscovery is used for and the after only includes routes, but routes are the main time in the first one, so it's possible to compare still.

Also note this is comparing a FileCache miss (on the basis that will happen on different web heads, between cli and web etc.) and a CacheCollector hit - because the goal here is better hit rates.

I got the 'before' via:

1. install standard.

2. Browse to admin/config/development/performance ready to clear the cache.

3. ddev restart && ddev xhprof on && ddev xhgui (so that apcu is empty)

4. Submit the cache clear form

For the after you just have to clear the cache via the form a couple of times.

Standard has less routes than Umami which has a lot less routes than most production sites, so I'd expect bigger savings in production.

Big advantage of this for routing is that the cache will be shared between drush cr building routes on the cli vs. something like saving a view, which also rebuilds the router.

On standard this saves about 190ms wall time and 350mb memory - but this is only applied to one of the caches on that request, there are lots of other candidates.

Had been wondering in the back of my head whether we should consider using hash_file instead of mtime for the file comparison, but the garbage collection is able to use the stored mtime and wouldn't be able to use hash_file() so probably better to stick with mtime, or at least we'd still need to store mtime either way or come up with a different garbage collection approach.

Went ahead and updated performance tests.

Per the issue summary, there are more places we can apply this to:

- Yaml cache collector can be used for menu links, local tasks, local actions, ckeditor5 plugins, breakpoints and more.

- we can add js/css minification caches

- attribute plugin cache

However some or all of these are follow-up material.

Updated performance tests - the updates show a single cache get after all cache bins are emptied, no set/lock etc. - this is showing the persistence working since the bin we use doesn't get cleared by the tests. The single cache get is a big different from the previous approach on this issue which has a cache entry per-file. Once we add it to more places the cache gets will go up by one per discovery type.

I think there's enough here for an initial review.

This is nice work and surprisingly straightforward.

I was also surprised especially given how complicated the previous attempt ended up before abandonment.

Also, should the issue title be updated to mention that this is for YAML discovery only?

Not sure about this yet.

I'm pretty sure we can apply it to css/js minification and attribute plugin discovery. Doing that here might make the scope a bit to big, but it would also allow us to add a base class or trait which encapsulates all the non-parsing logic for the cache collector (mostly the mtime checks) in a way that hopefully would result in less churn later. So.. I am thinking of trying to add support for one or the other or both, then if it feels a bit unwieldy, splitting it out after that.

If that turns into a bigger job than expected though, then yes we should only cover YAML here and just look at everything else in follow-ups.

I applied several of the suggestions, responded to a couple, and also added test coverage for the ::updateCache() override.

Or will the cache collector live only in the Core AttributeClassDiscovery subclass?

What I'm hoping is that we can do something very similar to the YamlCacheCollectorDiscovery added here - subclass with a couple of method overrides to change how the caching works and not too much else.

OK I started to look at attribute plugin discovery, the file parsing logic is a lot more complicated than YAML, so it's going to mean refactoring parts of the component attribute parsing out to a trait, then using those methods in a new core attribute discovery that shares the trait but instead inherits from the cache collector. It seems doable but too much to review in a single MR.

I've added the base class, so I think what I'll try is opening a separate draft MR for the plugin attribute parsing, that will validate whether the base class is flexible enough. We can then move those commits over to a new issue when this one is more or less ready.

On js minification when I opened this issue we hadn't done #3505776: Retain asset aggregates for 45 days by default to reduce generation of identical files, but that issue will mean a much lower cache rate here, so while it will still help for files duplicated between aggregates, or building an aggregate with 10 files and where only one changed, that's still a very low cache rate, and other asset changes in the queue should progressively make it even lower (by making the hit rate on aggregates much higher which is what we want). So going to hold off that bit for now.

Started the attributes branch.

Attribute parsing doesn't neatly fit into the cache collector API because you need more than the filename to parse the attribute, so the ::get()/resolveCacheMiss() pattern breaks down a bit. On the other hand it's easy to inline a bit of that logic into ::findDefinitions() and those other methods won't get called - probably need to BadMethodCallException from them.

I've done enough to the branch that it doesn't error, however it doesn't do dependency injection or bc properly yet and there are multiple layers of that to untangle (both the discovery and the various plugin managers will need various constructor bc layers). Haven't profiled it yet either. If it works enough to profile though, that should give us some numbers to look at - at which point can move the branch to its own issue and there should be some motivation to work on the constructor deprecations at that point.

Very initial profiling of attribute discovery changes - just the standard profile saving the clear caches form. The form itself doesn't cause a lot of plugin discovery compared to a busy front end page with entities etc. so not the biggest saving, but still visible.

One is a cache hit, the other is a cache miss (e.g. after TRUNCATE cache_file_parsing).

The big thing is we're saving about 450kb of memory on a cache hit - again we already get that when there's a filecache APCu hit, the idea is to have that more often.

I've opened #3591677: [PP-1] Add a persistent cache for plugin attribute parsing and moved the attributes MR progress over there, closed the one here.

Edit: and also #3591680: Use the YAML parsing cache collector for config file storage for another case where we parse a lot of YAML.

Pushed the change to pass in a yaml cache collector instead of three services.

Factory pattern - didn't do this yet because I'm not sure what's best - it might be worth making any factory format-specific rather than one factory for the various formats. So start with a yaml one and see if we need an attributes one etc.

Should be happier.

Re-titling to be a bit closer to what the new implementation is.

Rebased.

That one gets tricky due to the installer and container rebuilds so would definitely need to be it's own issue.

Rebased this one and added an comment to the service definition.

Added a change record.

Also pushed an 11.x backport.

Pushed a small follow-up commit to main to fix the namespace of a unit test - which failed hard in the 11.x backport MR.

Backport is green - only test changes and apart from silly mistakes the conflicts were easy to fix, so moving back to RTBC.