ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load()

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Reroll of #168, resolving two conflicts:

• In ContentEntityStorageBase::setPersistentCache, cache tags are now set using the $items array and $this->cacheBackend->setMultiple($items), rather than this->cacheBackend->set() as in #168.
• In SqlContentEntityStorageTest::testLoadMultiplePersistentCacheMiss, changed $entity->expects($this->any())->method('isDefaultRevision')->will($this->returnValue(TRUE)); to $entity->expects($this->any())->method('isDefaultRevision')->willReturn(TRUE);

The last submitted patch, 172: 2620980-172.patch, failed testing. View results →

Patch applies correctly to latest releases of both D9 and D10. (haven't tested dev)
We are using it to fix serious performance issues in a heavy customised layout builder setup. Layout builder pages can contains up to 100 inline blocks. (but really there is no actual hard limit for us)

Before the patch (while editing and translating pages with LB) we had 5-10.000 SQL queries taking 3-5seconds of loading time. (depending on the complexity of the layout builder setup on the page)
After the patch this went down to few hundred SQL queries and e few hundred ms of loading time.

This is a lifesaver for us, so thanks to everyone here for all the work put in!

I rerolled patch #172 and moved it into a MR, but it still needs work to get all the tests to pass.

I'm hiding the patches so that future fixes can continue in the MR.

Rerolled the patch to 10.3, since the current changes are aimed toward 11.x

Rerolled patch for 10.3.x

Started with a rebase.

📌 Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache() Active got in, which shows that on demo_umami on node pages, we have ~80 node revision queries due to the local tasks block, so that shows how valuable this would be:

     $expected = [
-      'QueryCount' => 172,
-      'CacheGetCount' => 247,
+      'QueryCount' => 94,
+      'CacheGetCount' => 250,

The reason for this is the active routes, which query and load the most recent revision for edting, instead of the default. I don't think we really understood (I certainly didn't) the cost of introducing that standardization around the getActive() and using it directly on routes. And then with umami you have edit + delete + translations + layout builder ...

Note that this happens even for anonymous users which do not have access to those routes. But upcasting happens before access checks.

But, this comes at a price with the current implementation, a massive increase of cache tag lookups:

-      'CacheTagLookupQueryCount' => 28,
+      'CacheTagLookupQueryCount' => 57,

I haven't looked at this in many years, but I do believe that this is doing too much and that held it back for that long. I will need to have a better understanding of the current implementation, but I strongly support #132 back from 2018 right now and to split this up and only do static caching first. That should essentially give us the same benefit in this specific scenario without the cost. Does that risk that we might never get persistent caching for revisions? Yes. But it also means that we might get static caching soon as opposed to never.

I've been working on "saving" the persistent cache and reducing the BC impact of this. There are a few interesting advantages of keeping the persistent cache, for example the optimization with loadRevisionUnchanged() that allows to load a revision from static cache and only bypass the static cache, which is very useful when saving an entity, and also just loading the current revision which is what's going on with getActive().

* I don't see a use case for invalidating a specific revision as an API. The cache should be self-contained, basically the only reason for external calls to resetCache() are tests and we've mostly removed that too thanks to automatically invalidating the memory cache.
* This allows to remove the default_revision_for cache tag, we should always have the entity ID available and then we can delete that from the cache directly.

The remaining cache tag is now revision_for.., that still shows up a lot in the performance tests, because of the optimization to also put the default revision with its revision ID into the cache, but that means we need to check the state of those cache tags on cache set.

I plan to look into either not doing that anymore for now or alternatively remove the cache tag. The number of revisions for an entity is a known quantity. So in cases where we need to invalidate them, we can query the ids and then invalidate the cache. I think that's not necessary that often, basically just when an entity is deleted?

Looks like the most recent changes broke something, will look into that, off for now.

Fixed the last non-performance test fails now I think. That one was interesting, it was reverting an old revision that was the the default revision back then to the new current revision. The logic here was to invalidate the loaded revision, but that wasn't actually the previous default revision. I extended it to also invalidate $entity->getOriginal()->getRevisionId() which works in this case because the revision we revert to was the default, so the logic in \Drupal\Core\Entity\ContentEntityStorageBase::doPreSave() does not kick in and it does _not_ use loaded revision as source. But if you would revert back to a draft then that wouldn't work. To fix this for that (non-tested) scenario, we'd need to reliably know the previous default revision id.

I'm also not 100% sure what makes sense as getOriginal() but I think it is in fact the current default revision, because that's what we're changing from.

Or, we drop this pretty complex optimization logic to avoid invalidating all old revisions when saving an entity. This would get a bit more expensive specifically when we'd combine it with the option in #183 to remove the cache tag and explicitly delete all known revision ids. But you'd probably need many thousands for this to really be an issue.

The remaining cache tag is now revision_for.., that still shows up a lot in the performance tests, because of the optimization to also put the default revision with its revision ID into the cache, but that means we need to check the state of those cache tags on cache set.

I plan to look into either not doing that anymore for now or alternatively remove the cache tag

Could we use the entity:$entity_id cache tag instead? Feels like most pages that would request it would do so anyway, and in other situations it wouldn't be much overhead.

If we can get rid of it by reversing "because of the optimization to also put the default revision with its revision ID into the cache," that also seems fine.