#Performance

Did some manual testing of this + together.

This is what the file_cache table looks like after running the UI installer with the standard profile:

MariaDB [db]> SELECT COUNT(*) FROM file_cache WHERE cid LIKE '%\:hook_implementations%';
+----------+
| COUNT(*) |
+----------+
|       62 |
+----------+
1 row in set (0.001 sec)

MariaDB [db]> SELECT COUNT(*) FROM file_cache WHERE cid LIKE '%:procedural_hook_implementations%';
+----------+
| COUNT(*) |
+----------+
|     1591 |
+----------+
1 row in set (0.002 sec)

As you can see there are far fewer entries for the non-procedural implementations, this is because we don't have to use the module regexp in the cache key so they persist across module installs, whereas procedural implementations are discovered each time to allow for implementing hooks from other modules.

I pushed one additional commit:

1. Gave the procedural hook implementation items their own namespace, this made it easier to run the queries above to differentiate the prevalence of the two. Due to the module preg sharing the namespace was fine, but nice to see what things look like.

2. Removed the deployment identifier from the cache key - it's already in apcu_prefix so this would mean it's in the cid twice.

Leaving RTBC because these are very small changes.

The new test coverage looks good, marked threads on the MR resolved because I agree with the answers given.

Tagging beta target because this should significantly help mitigate the increased memory usage due to the procedural hook bc layer.

I added a procedural test too.

Ok this is ready for review.

I ran the test only, but that won't fail cause the current code and previous code both work for the situation, but it will prevent the regression.

I'm taking a crack at this.

@duaelfr I like this idea. It is similar to how domain_entity module handles things.

This feature would really help me out ; )

Wonder if it's better to create a new cache backend.

Thanks @quietone, from nxt time will keep it in mind...

Relevant conversation: https://www.reddit.com/r/PHP/comments/vlyw1g/benchmarking_serialization/

I did some benchmarking locally. Getting cached items back is almost instance when using VarExporter.

Here are some results of profiling Drupal site with cache.container implemented with PhpBackend

Here are the results:

For local Drupal 11 installation the performance boost was about 5-10% for anonymous visitors.

@sourav_paul, For Drupal core, it is preferred that contributors add a comment that they are working on an issue instead of assigning it to themselves. See Assigning ownership of a Drupal core issue → .

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

Can you share proper reproducing steps?

Given we broke contrib let's add a test to ensure we don't regress again in the future

Tests are passing in XB with this change now. https://git.drupalcode.org/issue/experience_builder-3487815/-/jobs/3373246

I created 📌 [ignore] Test Filecache Active we can apply this there

These changes look good to me actually, can we test this against XB before going through the commit process again?

Ahh #43 is a good point, however I think we're OK here because those implementations will still be discovered by the OOP attribute discovery (just not invoked when hooks are invoked), whereas the problem we have with experience_builder is the procedural implementation not being discovered at all until the module is installed. There's no way to tell which module foo_bar_foo_bar_foo_bar() is implemented on behalf of, until foo, foo_bar or foo_bar_foo module is installed, but the attribute is explicit.

Just a quick note, you can implement on behalf of other modules using #[Hook('toolbar_alter', module: 'module_on_behalf_of')]

You can't implement on behalf of a theme though.

catch → reopened merge request !10115

Not sure what the fix is yet, maybe we need to hash the regex as part of the cache key?

The concept of implementing a hook on behalf of another module doesn't exist with OOP hooks (you can check moduleExists() inside a hook implementation of course still), so we only need to cover this with procedural hooks, so I tried implementing it only for them - seems simple enough but see what the pipeline says. The advantage if we do it this way is we only take the hit on the bc layer and only when the module list changes, so regular drush crs still get the benefit as do converted modules.

catch → changed the visibility of the branch 3478621-add-a-filecache-backed to active.

That might work, but can we also just dump it on module install?

We won't see as much benefit in that case as we'd want, but that should solve this case for now right?

Reverted, this broke Experience Builder and will break many other modules that implement hooks on behalf of others.

Experience Builder implements two hooks on behalf of core modules: datetime_range_storage_prop_shape_alter and media_library_storage_prop_shape_alter - this is so the changes only take effect if those modules are installed.

During a test, we first of all install without media_library or datetime_range enabled. The hook discovery runs and these two hook implementations are not cached, because they aren't in the modules list and therefore aren't included in the function name regex. Later in the test, we install these modules, but the hook implementation cache is not cleared; the code believes it's already discovered all the hooks for experience_builder.module, the cached result is used, and the hooks are never found or executed at runtime.

This will presumably affect any module that implements hooks on behalf of other modules, the incomplete set of hooks may be cached and then never cleared. Not sure what the fix is yet, maybe we need to hash the regex as part of the cache key?

longwave → closed merge request !10115

Let's squeeze this into 11.1.0-beta1.

Committed and pushed 6aca2acfd19 to 11.x and f6f705f8a29 to 11.1.x. Thanks!

Oh great! Looks good to me still and you addressed @longwave's comment on the mr.

Rtbc from me again

No it is that, I just failed to push the correct branch.

I would just require drush locally and search for ckeditor5_config_schema_info_alter. If that doesn't find it I'll investigate further when I'm at my desk in about an hour

OK it's not that, might be to do with config_inspector but I can't see any direct hook invocations, which leaves 'something weird' or drush.

I think that is specific to this MR and that job. The 11.x branch tip here was from before OOP hooks so it was getting very confused. I've pushed a rebase of the 11.x branch here and will re-run the job.

Oh that looks like we missed a direct call to a hook, I can take a look in a bit.

It could also be in drush or another vendor package.

I recently discussed thie with catch.

Using the header and vary on that is very dangerous for cache purposes. It will make very very many variations, which results in bad caching and performance. Not to mention possible memroy issues in caches or high cache trashing.

We did think about how we could possible work around that.

What if;

We normalize the prermutation of the language into the resulting language, and cache that normalization and the resulting language.

So perhaps a site with en_EN and fr_FR. A browser comes along and tells the site "Hi i'm en-GB,en-US;q=0.9,en;q=0.8". The site would find out what this will end up as so

en-GB,en-US;q=0.9,en;q=0.8 magic => en_EN, we cache this and threat serve cache asif en_EN. This would mean (at least on the drupal side) only the records for inLang => outLang exist, which are pretty small and we dont end up with loads of difference permutations.

Not entirely sure how we would implement that, but feels like a way to have drupal cache be happy.

The little devil on my shoulder also has a voice though. Which does tell me this is kinda a. can of worms as this could also easily break caching on the caches in front of Drupal, be it varnisch or maybe smoething like cloudflare. That is kinda the scary part. Perhaps contrib is better to have this live, instead of trying to get this into core.

The "validatable config" job is failing on drush cr, unsure if it's directly related to this but looks relevant.

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

$ vendor/bin/drush cr --quiet
Error: Call to undefined function \ckeditor5_config_schema_info_alter() in /builds/project/drupal/core/lib/Drupal/Core/Extension/ModuleHandler.php on line 459 #0 /builds/project/drupal/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(386): Drupal\Core\Extension\ModuleHandler->alter('config_schema_i...', Array)
#1 /builds/project/drupal/core/lib/Drupal/Core/Config/TypedConfigManager.php(406): Drupal\Core\Plugin\DefaultPluginManager->alterDefinitions(Array)

Fixed PHPCS, only rearranging lines and deleting some blank ones.

Good point - pushed a commit for this, since it's a trivial change, moving back to RTBC.

The changes make sense, there appears to be a further optimisation we can do but otherwise this looks good.

The original issue title was overly optimistic, maybe we can come up with a generic filecache-backed attribute parser but I think we should do a couple of one-off issues like this before that to understand the requirements better.

That is pretty clever!

Tagging this RTBC with the assumption we don't need to test individual cache sets and gets.

If we need that let me know.

This does have a lot of implicit coverage, if the cache wasn't storing it properly everything would break.

Feels obvious now that you've pointed it out - the file cache depends on the filename and can't use arbitrary cids.

However, when I went to remove it, I realised we can probably add it to the namespace instead - seems like this should work but let's see what the pipeline says.

We can't use the Settings::get('deployment_identifier') for the file cache since it relies on the actual file to get the mtime.

Because of the major shift in 📌 Replace book_node_builder with entity fields Active I'm considering starting a 3.0.x branch

✨ Make book child content-type configurable per enabled book content-type Postponed I would like to land in 2.0.x though and think it's pretty safe if we land the update hook right.

IMHO if you're building a site now it's worth starting with v2 and get used to its (limited) changes, rather than having to deal with it later.

Hm. Good to know. We are trying to decide if we want to move from core to 1.x (which is a full release covered by the security policy, we also need to bring some patches with, but still fails some of our caching tests) or to 2.x (which we don't need the patches for, but might have more changes that need more QA). I'm unsure.