Add static and persistent caching to ContentEntityStorageBase::loadRevision()

+1. When entity caching was added originally, we didn't have separate methods for loading revisions. This is also why #1730874: Add support for loading multiple revisions at once wasn't implemented for revisions at the same time (i.e. node_load(1, 2) was the API to get node 1 at revision 2. Given the workflow intiative there'll be a lot more need for loading and viewing revisions - as well as the contrib modules that reference specific revisions (although I'm skeptical about that except for very specific cases).

Haven't reviewed the patch yet.

mostly a reroll, the change to the use statements is already in core now.
Sorry, I misread the rejects. Will post an updated patch.

reroll of #19

reroll of #19

Should we profile this as well?

After working on #2809227: Store revision id in originalRevisionId property to use after revision id is updated I found that most of the stuff is already in the patch from #27 so I've been looking at fixing the failing tests.

The only failing test that matters is \Drupal\Tests\content_moderation\Kernel\ContentModerationStateTest and this seems to be due to where we load the ContentModerationState entity in \Drupal\content_moderation\Plugin\Field\ModerationStateFieldItemList::getModerationState.We use $storage->loadRevision(), but $content_moderation_state->fields['moderation_state'] and $content_moderation_state->values['moderation_state'] seem to have different values.

Think I'm getting closer to what's causing the issue here:

    // Change the state without saving the node.
    $content_moderation_state = ContentModerationState::load(1);
    $content_moderation_state->set('moderation_state', 'draft');
    $content_moderation_state->setNewRevision(TRUE);
    $content_moderation_state->save();

In this part of the test we load the ContentModerationState entity, change the moderation state.

  public function save() {
    $related_entity = \Drupal::entityTypeManager()
      ->getStorage($this->content_entity_type_id->value)
      ->loadRevision($this->content_entity_revision_id->value);
    if ($related_entity instanceof TranslatableInterface) {
      $related_entity = $related_entity->getTranslation($this->activeLangcode);
    }
    $related_entity->moderation_state->target_id = $this->moderation_state->target_id;
    return $related_entity->save();
  }

then in the save method we don't actually save the ContentModeration state, we save the parent entity. Therefore the ContentModerationState where we changed the moderation state is still cached with the wrong information.

This should fix it.

@hchonov - That produces a new error, will dig into why:

1) Drupal\Tests\content_moderation\Kernel\ContentModerationStateTest::testBasicModeration
Error: Call to a member function isPublishedState() on null

/home/timmillwood/Code/drupal/core/modules/content_moderation/src/EntityOperations.php:120
/home/timmillwood/Code/drupal/core/modules/content_moderation/content_moderation.module:90
/home/timmillwood/Code/drupal/core/lib/Drupal/Core/Extension/ModuleHandler.php:402
/home/timmillwood/Code/drupal/core/lib/Drupal/Core/Entity/EntityStorageBase.php:169
/home/timmillwood/Code/drupal/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:502
/home/timmillwood/Code/drupal/core/lib/Drupal/Core/Entity/EntityStorageBase.php:435
/home/timmillwood/Code/drupal/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:359
/home/timmillwood/Code/drupal/core/lib/Drupal/Core/Entity/EntityStorageBase.php:389
/home/timmillwood/Code/drupal/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:761
/home/timmillwood/Code/drupal/core/lib/Drupal/Core/Entity/Entity.php:364
/home/timmillwood/Code/drupal/core/modules/content_moderation/src/Entity/ContentModerationState.php:159
/home/timmillwood/Code/drupal/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php:75

So $related_entity->moderation_state->entity = $this; as suggested in #38 won't ever work because we need to set the moderation state to a ModerationState entity, not a ContentModerationState.

Therefore $related_entity->moderation_state->entity = $this->moderation_state->entity; or $related_entity->moderation_state->target_id = $this->moderation_state->target_id; (as set in #35) are the only options. Although both if these require a cache reset.

We have a ContentModerationState which has an entity reference field to a ModerationState config entity, it also references the entity the moderation state is for (such as a node or block) but not via entity reference field.

In the test we change the moderation state of an entity by updating it's ContentModerationState. So load revision X of ContentModerationState, and change the ModerationState reference from Y to Z. When staving the ContentModerationState it doesn't actually save it just updates the $related_entity->moderation_state->entity. So next time revision X of ContentModerationState is loaded it still has ModerationState Z and not Y as it should.

It'd like to use getOriginalRevisionId() to fix a bug in 8.2.x, so switching this to 8.2.x, but not sure it's a valid patch release patch,

Addressed items from #44

I think it's not applying because the patch was for 8.3.x and this issue was for 8.2.x

Ideally I'd like to see this in 8.2.x, but I can't see that happening.

For #44.1 I needed to add these methods back in more than just the interface.

In general, how does this play with #1596472: Replace hard coded static cache of entities with cache backends? Would it buy us anything if we do that issue first? Or would that one be harder to do if we go with this one first?

Skimmed a bit through the patch and a few points stand out:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -301,6 +369,22 @@ protected function doPostSave(EntityInterface $entity, $update) {
   +      // If a non-default revision has been loaded it has been statically only
   +      // in the entity revision cache, which means that the parent class call
   +      // in ::doPostSave to ::resetCache will not be able to reset the static
   +      // revision cache as it is only calling the function with the entity id,
   +      // but as for this entity id there will be no entity cache entry we'll
   +      // not be able to retrieve the revision id and remove its static cache
   +      // there.
   +      // @todo consider adding an optional entity parameter to ::resetCache, as
   +      // in this case we'll not need this logic here.
   +      $this->resetRevisionCache([$entity->getRevisionId(), $entity->getOriginalRevisionId()]);
   

   Wouldn't it be easier if we would key the static revision cache by entity ID first and then revision ID (i.e. $entityRevisions[$entity_id][$revision_id])?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -8,6 +8,52 @@
   +   * @param int|string $revision_id
   +   *   The revision id.
   ...
   +   * @param mixed $revision_id
   +   *   The revision ID of the entity to load.
   

   Core assumes integer revision IDs in the storage class, so I think we can make the same assumption here and everywhere else we deal with a revision ID.

3. +++ b/core/lib/Drupal/Core/Entity/OriginalRevisionIdInterface.php
   @@ -0,0 +1,30 @@
   +   * @return
   +   *   The original revision identifier of the entity, or NULL if the entity
   +   *   does not have a revision identifier.
   +   */
   +  public function getOriginalRevisionId();
   

   Related to the point above, we're missing the return type here.

I had not seen #1596472: Replace hard coded static cache of entities with cache backends, maybe it'd be better to revisit that? and in the mean time I go back to #2809227: Store revision id in originalRevisionId property to use after revision id is updated and just implement the getOriginalRevisionId() elements from this issue?

Not sure if it matters in which order we get those issues in, the second will need a reroll but not sure which way round it will be harder.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -219,6 +226,10 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
   +
   +    // Store the original revision identfier the entity has been loaded with to
   +    // keep it safe from changes.
   +    $this->originalRevisionId = $this->getRevisionId();
   

   should we only do this if we have a revision key? getEntityKey() will check it too, but it also checks first if it already has a key.

   Not really for performance, more for reading the code.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -21,6 +21,13 @@
   +   * Static cache of entity revisions, keyed by entity revision ID.
   +   *
   +   * @var array
   +   */
   +  protected $entityRevisions = [];
   

   this would then simply change to use the corresponding static cache class.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -215,7 +222,68 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +    // If there is already an entry in the static entity revisions cache we
   +    // return a reference to the already loaded entity object.
   +    //
   +    // @todo find a better way to determine the default revision id without
   +    // having to load the entity object from the storage, then check if there
   +    // is already an entry in the static entity revisions cache for it and if
   +    // not load the entity from the storage.
   +    if ($entity && $this->entityType->isStaticallyCacheable() && $this->entityType->isRevisionable()) {
   

   The first part of the comment doesn't really make sense to me.

   I understand this exists to prevent that loading an entity by revision and then by ID or the other way round always returns the same, statically cached object?

   About the @todo, what if we do what @amateescu suggested and cache it as [entity-id][revision-id]. If we then additionally do a special case for the default revision and cache that as [entity-id]['default'] instead of the actual ID, then we could actually directly return that if we have it?

Just in general #1596472: Replace hard coded static cache of entities with cache backends has a working patch and needs reviews, and in turn would allow migrations to stop clearing persistent entity caches when they reclaim memory once the dependent LRU issue is in.

After talking on IRC today we decided to implement the getOriginalRevisionId() part of this issue in #2809227: Store revision id in originalRevisionId property to use after revision id is updated, then come back to this.

Here's a patch without the new methods and the interface. This can be used after the other gets in.

I applied the patch from #59 on top of the latest patch in #2809227: Store revision id in originalRevisionId property to use after revision id is updated and all seemed to work ok.

Thanks for the re-roll some observations.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -215,13 +222,81 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +    // @todo find a better way to determine the default revision id without
   +    // having to load the entity object from the storage, then check if there
   +    // is already an entry in the static entity revisions cache for it and if
   +    // not load the entity from the storage.
   

   This can be removed now after #2809227: Store revision id in originalRevisionId property to use after revision id is updated.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -677,21 +806,105 @@ public function loadUnchanged($id) {
   +   * Resets the internal, static entity revision cache.
   

   'internal' seems redundant here.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -677,21 +806,105 @@ public function loadUnchanged($id) {
   +      $this->ignoreStaticCache = TRUE;
   +      $revision = $this->loadRevision($revision_id);
   +      unset($this->ignoreStaticCache);
   

   How about restoring the ignoreStaticCache after the loadRevision?

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -707,4 +920,38 @@ protected function buildCacheId($id) {
   +   * @param int $id
   +   *   The entity id.
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -8,6 +8,52 @@
   +   * @param mixed $id
   

   It should be string|int instead.

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -707,4 +920,38 @@ protected function buildCacheId($id) {
   +   * @return int|null
   +   *   The default revision id.
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -8,6 +8,52 @@
   +   * @param int|string $revision_id
   +   *   The revision id.
   ...
   +   * @param mixed $revision_id
   +   *   The revision ID of the entity to load.
   ...
   +   * @param int $revision_id
   +   *   The revision id.
   

   AFAIK revision ID can only be int. Can a revision ID be string?

6. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -707,4 +920,38 @@ protected function buildCacheId($id) {
   +    else {
   +      $result = NULL;
   +    }
   

   If we initialize $result with NULL then we don't need this 'else'.

7. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -8,6 +8,52 @@
   +   * @todo Remove this method once we have a reliable way to retrieve the
   +   *   unchanged entity from the entity object.
   

   Can't we do this after #2809227: Store revision id in originalRevisionId property to use after revision id is updated?

8. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -14,7 +14,7 @@
   -  protected $entities = array();
   +  protected $entities = [];
   

   Just increasing the patch size so let's not do this.

9. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueContentEntityStorage.php
   @@ -13,6 +13,27 @@ class KeyValueContentEntityStorage extends KeyValueEntityStorage implements Cont
   +  public function loadRevision($revision_id) {
   ...
   +  public function deleteRevision($revision_id) {
   

   We can use the parent methods and return NULL;.

10. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueContentEntityStorage.php
    @@ -13,6 +13,27 @@ class KeyValueContentEntityStorage extends KeyValueEntityStorage implements Cont
    +  public function loadRevisionUnchanged($revision_id) {
    +    // @todo
    +  }
    

    return NULL; maybe?

11. +++ b/core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php
    @@ -47,7 +47,7 @@ protected function setUp() {
    +    $this->storage = $this->prophesize('\Drupal\Core\Entity\ContentEntityStorageInterface');
    
    @@ -60,7 +60,7 @@ protected function setUp() {
    +    $entity = $this->prophesize('\Drupal\Core\Entity\ContentEntityInterface')
    

    Let's use class constants.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -215,13 +232,81 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +      return $this->getFromStaticEntityRevisionCache($entity->getRevisionId());
   
   @@ -692,24 +831,108 @@ public function loadUnchanged($id) {
   +   * @return \Drupal\Core\Entity\ContentEntityInterface|NULL
   +   *   The entity from the entity revision cache or NULL if it is not present.
   +   */
   +  protected function getFromStaticEntityRevisionCache($revision_id) {
   

   This is problematic because getFromStaticEntityRevisionCache() can return NULL. In general I'm not sure why we try to fetch something from the cache when we already have an entity that we could just return.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -692,24 +831,108 @@ public function loadUnchanged($id) {
   +    // Update the default revision
   +    if ($entity) {
   +
   +    }
   

   What is this?

Just found one more reason why the return $this->getFromStaticEntityRevisionCache($entity->getRevisionId()); is wrong:

Inside $storage->loadUnchanged() we end up calling $this->load(), but before that we clear the static cache by calling parent::resetCache(). But we don't clear the static revision cache. So the call to $this->getFromStaticEntityRevisionCache() that then happens in load() (inside loadUnchanged()) will potentially re-use a statically cached revision that might have already been tampered with before. So it fundamentally breaks the contract of loadUnchanged().

Note that this particular issue could be fixed by explicitly clearing the static revision cache in loadUnchanged() below the parent::resetCache() call.

I think it is important that we add test coverage to expose #73 before trying to fix it. I tried to cook up something locally but failed so far, so will try to explain what happened:

An entity was being saved with an updated field value. As part of saving $entity->original was being populated with the return value from loadUnchanged(). Because of the above behavior, loadUnchanged() actually returned the already modified entity (this part I could not yet isolate in a test, see below). Then when saving the dedicated table data, Drupal sees if anything changed for the given field, but since $entity->original was actually identical to $entity, Drupal ignored this field and did not persist the values to the database, even though the field values were (both) different from the ones in the database.

So in other words:

// Do something magical, which I have not been able to isolate
$entity->foo->value = 'bar';
$entity->save();
$entity = Entity::load($entity->id());
print $entity->foo->value;

This will print the value that the entity had before, and not 'bar'.

Regarding not being able to isolate this behavior, you need to create a situation where the entity that you have modified is in the static cache but the persistent cache does not have that entity. Then we will reach $this->load() in ContentEntityStorageBase::loadUnchanged() and the craziness begins...

First a re-roll for #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase. Working on a test again now.

This will not happen, because before we retrieve the entity from the static cache we first check if it is there and if not we put it into the static cache. This happens just two rows before the one you are referencing to.

It still seems unnecessary, though. Why not just return $entity;?

See also #2859042: Impossible to update an entity revision if the field value you are updating matches the default revision. for a similar problem that exists in HEAD with forward revisions where changes aren't stored. That issue might also need this, although I'm not sure if we should implicitly rely on the static caching here, we might want to make it more explicit.

Tempted to RTBC this, but it's a little over my head.

So just going to add a "looks good to me" comment.

Few review comments:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -215,13 +232,81 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +      if (!$this->getFromStaticEntityRevisionCache($entity->getRevisionId())) {
   +        $this->setStaticEntityRevisionCache($entity->getRevisionId(), $entity);
   ...
   +      return $this->getFromStaticEntityRevisionCache($entity->getRevisionId());
   

   Minor: We could avoid 2 additional calls to getRevisionId by storing in local variable.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -628,6 +726,44 @@ protected function getFromPersistentCache(array &$ids = NULL) {
   +    if ($revision) {
   +      $entities = [$revision->id() => $revision];
   ...
   +      $this->postLoad($entities);
   
   @@ -710,6 +855,129 @@ public function resetCache(array $ids = NULL) {
   +    if ($revision = $this->getRevisionFromPersistentCache($revision_id)) {
   ...
   +      $this->postLoad($entities);
   

   getRevisionFromPersistentCache already doing a postLoad() do we still need one here?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -710,6 +855,129 @@ public function resetCache(array $ids = NULL) {
   +    $entity = $this->entityType->isStaticallyCacheable() && isset($this->entityRevisions[$revision_id]) ? $this->entityRevisions[$revision_id] : NULL;
   ...
   +    return $entity;
   

   can be single line return statement.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -710,6 +855,129 @@ public function resetCache(array $ids = NULL) {
   +    // Update the default revision
   +    if ($entity) {
   +
   +    }
   

   empty if block

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -8,6 +8,52 @@
   +  public function load($id);
   

   load() is already in EntityStorageInterface. Do we need to redefine here?

6. +++ b/core/lib/Drupal/Core/Entity/KeyValueStore/KeyValueContentEntityStorage.php
   @@ -13,6 +13,30 @@ class KeyValueContentEntityStorage extends KeyValueEntityStorage implements Cont
   +    //   https://www.drupal.org/node/2618436.
   ...
   +    //   https://www.drupal.org/node/2618436.
   ...
   +    //   https://www.drupal.org/node/2618436.
   

   More than 2 spaces.

Thanks @hchonov. interdiff looks good to me. I would like to do a manual testing to make sure getting proposed resolution. If it's too much to ask/do "steps to reproduce" in issue summary (as I understand it's more of technical fix than UI change), can we have a test-only patch to prove it still fails?

Patch no longer applies.

And a few minor issues:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -215,13 +232,81 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +    // @todo find a better way to determine the default revision id without
   +    // having to load the entity object from the storage, then check if there
   +    // is already an entry in the static entity revisions cache for it and if
   +    // not load the entity from the storage.
   

   Any @todo needs a corresponding issue linked from the code comment.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -215,13 +232,81 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
   +   * @param int|string $revision_id
   +   *   The revision id.
   

   Can the revision_id really be a string? Elsewhere in the patch it's just an int...

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -323,6 +408,19 @@ protected function doPostSave(EntityInterface $entity, $update) {
   +      // @todo consider adding an optional entity parameter to ::resetCache, as
   +      // in this case we'll not need this logic here.
   

   Ditto here.

Straight reroll for the minute. Looking at the feedback in #89 and reviewing again shortly.

Would it be worth adding a test case for the scenario in #91 so a few of these options can be evaluated.

Not sure why in #90 the method isDefaultRevisionPublished was copied from ModerationInformation to EntityOperations.

So for now a re-roll without that move.

Still open #89 and #91

We already have a recursion protection in EntityOperations::entityLoad(), which is a bit ugly but it will be removed by #2924218: Clearing the persistent entity cache every time we switch between workspaces is super wasteful, so I would suggest to either wait for that issue to land or re-work this patch on top the latest one over there so you don't waste too much time chasing soon-to-be-dead code :)

Started reviewing, got about a third of the way through. Most notably, this patch now needs to be adapted for #1596472: Replace hard coded static cache of entities with cache backends.

Some more comments:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -35,6 +42,19 @@
   +   * without using the static entity cache or static entity revision cache.
   +   * @see ::loadUnchanged()
   +   * @see ::loadRevisionUnchanged()
   

   There should be an empty line before the @see's. Also, I don't think we're allowed to use omit the class name even if it's a reference within the same class.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -446,6 +466,41 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
   +    // If the entity is not present in the static entity cache then we check if
   +    // the default revision for the given entity ID is present in the static
   +    // entity revision cache.
   

   This kind of seems the wrong way around. Couldn't we instead - whenever we put a revision in the revision static cache check if it is the default revision and then populate the entity static cache directly?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -459,26 +514,63 @@ public function loadRevision($revision_id) {
   +    $passed_revision_ids = !empty($revision_ids) ? array_flip($revision_ids) : FALSE;
   

   I know this is mostly copied from ::loadMultiple(), so feel free to ignore, but wanted to mention it nonetheless.

   • Calling a processed version of a passed argument $passed_* is a bit strange, in my opinion.
   • The !empty() could be omitted, although I guess some people like keeping it around.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -459,26 +514,63 @@ public function loadRevision($revision_id) {
   +      // If any entities were loaded, remove them from the IDs still to load.
   +      $revision_ids = array_keys(array_diff_key($passed_revision_ids, $revisions));
   

   The equivalent code in ::loadMultiple() contains a check for (the equivalent of) if ($passed_revision_ids) here. Unless I'm missing something, that is in fact pointless, so that it's fine to omit it here, but I think we should then also remove it from ::loadMultiple(). I'm fine with doing that in a follow-up if you don't want to do that here.

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -459,26 +514,63 @@ public function loadRevision($revision_id) {
   +    // Load any remaining entities from the database. This is the case if there
   +    // are any revision IDs left to load.
   +    $queried_revisions = $this->doLoadMultipleRevisionsFieldItems($revision_ids);
   

   Here, again, ::loadMultiple() has a check for (the equivalent of) if ($revision_ids === NULL || $revision_ids) and here, it actually is a performance improvement to avoid that call, unless I'm missing something.

6. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -459,26 +514,63 @@ public function loadRevision($revision_id) {
   -      $this->invokeStorageLoadHook($entities);
   

   Why can we remove this? It seems it should still be called, at least on the queried entities?

7. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -608,8 +700,67 @@ protected function doPreSave(EntityInterface $entity) {
   +        // If the entity has been saved into a new revision, then we only have
   +        // to remove the original one from the static cache only. Otherwise we
   +        // have to delete the persistent cache as well.
   +        $this->resetStaticRevisionCache($revision_ids);
   

   The comment doesn't match the code, or at least it's written in an unclear way. Per the comment I would think we only have to clear the static revision cache if a new revision was created but it is cleared unconditionally.

   Also, the first sentence contains "only" twice.

8. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -608,8 +700,67 @@ protected function doPreSave(EntityInterface $entity) {
   +            // Exclude the revision ID field explicitly as it is not flagged as
   +            // revisionable.
   

   We could add a @todo to remove this in https://www.drupal.org/project/drupal/issues/2975307

9. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -608,8 +700,67 @@ protected function doPreSave(EntityInterface $entity) {
   +                  $current_langcode = $entity->language()->getId();
   +                  if ($original->hasTranslation($current_langcode)) {
   +                    $original_translation = $original->getTranslation($current_langcode);
   +                    if (!$original_translation->get($name)->equals($entity->get($name))) {
   +                      $changed_non_rev_field = TRUE;
   +                      break;
   +                    }
   +                  }
   +                  foreach (array_diff(array_keys($entity->getTranslationLanguages()), [$current_langcode]) as $langcode) {
   +                    if ($original->hasTranslation($langcode)) {
   +                      $original_translation = $original->getTranslation($langcode);
   +                      $translation = $entity->getTranslation($langcode);
   +                      if (!$original_translation->get($name)->equals($translation->get($name))) {
   +                        $changed_non_rev_field = TRUE;
   +                        break 2;
   +                      }
   +                    }
   +                  }
   

   Seems unnecessary to do this twice, can we not just include $current_language in the second loop?

   Also, I think the if ($original->hasTranslation($langcode) is missing an else. If the default revision adds a new translation we should always clear the persistent revision cache for that revision, no?

   All this leads me to wonder, whether this complex check is even wort it or whether it's not easier to just always clear the persistent revision cache in this case even though there might be some cases where it's not necessary.

10. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -922,6 +1093,19 @@ protected function getFromPersistentCache(array &$ids = NULL) {
    +    // We might have saved already a new entity revision as the default one so
    +    // on fetching from the persistent cache we have to update the default
    +    // revision status of the entity revisions.
    

    This seems pretty unfortunate. Wouldn't it be nicer to simply clear the persistent revision cache of the default revision when saving a new revision as the default revision?

11. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -940,8 +1124,23 @@ protected function setPersistentCache($entities) {
    +      if ($this->entityType->isRevisionable()) {
    +        if ($entity->isDefaultRevision()) {
    +          $this->cacheBackend->set($this->buildCacheId($entity->id()), $entity, CacheBackendInterface::CACHE_PERMANENT, $cache_tags);
    ...
    +      else {
    +        $this->cacheBackend->set($this->buildCacheId($entity->id()), $entity, CacheBackendInterface::CACHE_PERMANENT, $cache_tags);
    +      }
    

    I think isDefaultRevision() should return TRUE even if the entity type is not revisionable, so I think we can consolidate those two calls.

12. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -991,22 +1196,194 @@ public function loadUnchanged($id) {
    +      // There is no need to reset the persistent entity revision cache. This
    +      // normally is done only in the following cases:
    +      //  - an entity is deleted.
    +      //  - an entity revision is deleted.
    +      //  - a default entity revision with changes in non-revisionable fields is
    +      //    saved.
    +      //  - a non-default entity revision is saved in the same revision.
    

    I'm not sure I agree with that. Even though this is of course to be avoided generally people may be modifying the database directly.

    My thinking is that this would mostly be true for the non-revision persistent cache, as well. I.e. if the default revision does not get saved or deleted, we shouldn't have to clear the persistent cache. We do, though, unconditionally, so I think we should do the same for the revision cache.

13. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -991,22 +1196,194 @@ public function loadUnchanged($id) {
    +  protected function getLoadedRevisionIDsOfStaticallyCachedEntities(array $ids = NULL) {
    

    Since this method is only ever used in conjunction with ::resetStaticRevisionCache() I wonder whether instead of this method we should just add a ::resetStaticRevisionCacheByIds() method or similar (or maybe even a flag to ::resetStaticRevisionCache()?

14. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -1020,4 +1397,63 @@ protected function buildCacheId($id) {
    +  protected function buildRevisionCacheId($revision_id) {
    

    Since this is often used conditionally instead of ::buildCacheId() what do you think about instead of a new method, adding a $revision = FALSE parameter to buildCacheId() instead? That way we could simply pass down the $revision parameter in a bunch of the methods above.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -991,22 +1196,194 @@ public function loadUnchanged($id) {
   +        $revision_ids = array_filter($ids);
   

   Interesting, why is this necessary?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -991,22 +1196,194 @@ public function loadUnchanged($id) {
   +        $entities += array_intersect_key($this->entityRevisions, array_flip($ids));
   

   Seems we can just assign (=) here, instead of adding (+=).

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -991,22 +1196,194 @@ public function loadUnchanged($id) {
+  protected function getLoadedRevisionIDsOfStaticallyCachedEntities(array $ids = NULL) {
+    $revision_ids = [];
+    if (!$this->entityType->isRevisionable()) {
+      return $revision_ids;
+    }
+    $entities = isset($ids) ? array_intersect_key($this->entities, array_flip($ids)) : $this->entities;
+    /** @var \Drupal\Core\Entity\ContentEntityInterface[] $entities */
+    foreach ($entities as $entity) {
+      $revision_ids[] = $entity->getLoadedRevisionId();
+    }
+    return $revision_ids;
+  }

I just found a major problem with this patch: This method needs to take into account not only $this->entities but also $this-entityRevisions. It can be the case that there are statically cached revisions that are not also cached in $this->entities and in that case those revisions will never be cleared using resetRevisionCache()

What is also problematic is that if you have multiple revisions of the same entity in the revision cache you cannot clear all of the revisions using the API. Only the revision that happens to be the "loaded revision" will be cleared. I think there should be some way to explicitly clear by revision ID.

Had an in-person chat with @hchonov and @mkalkbrenner about this yesterday. Here are some notes. After writing this I will attempt to re-implement this (i.e. not re-roll in the strict sense) on top of current 8.7.x and the MemoryCache implementation.

• Just like the patch already re-uses the persistent cache backend, we can now re-use the MemoryCache for the static revision cache
• We discussed what the reasonable expectation would be with regards to calling EntityStorageInterface::resetCache($ids). With the current patch only the default revision is cleared from the revision cache. I.e. after calling $storage->resetCache([5]) there might still be a revision in the revision cache with the entity ID 5.
  1. One possible expectation could be that all revisions with that ID are cleared from the revision cache. This is what #112 was about (although that might be invalidated below).
  2. Clearing all revisions for a given ID might be needlessly clearing valid cache entries. If you have an entity with a lot of revisions in a standard editorial workflow, each time that entity is saved all revisions would be cleared from the persistent cache even though those historical revisions will never be changed. On the other hand, they probably won't be loaded/viewed all to often, so clearing the cache isn't necessarily terrible, it's just unnecessary in this case.

  I think backwards-compatibility is the most important thing here, which in this case means not to break the existing expectations of users. However, one could argue this for both implementations:

  1. Because resetCache() is the only method to clear an entity's (or entity revision's!) cache right now, we need to make sure that it continues to clear everything related to the entity including all of its revisions
  2. Because there currently is no revision cache, resetCache() should not mess with the newly introduced revision cache as that would be expanding its functionality. We just need to clear the default revision to prevent that from being used when subsequently calling load()
• Regardless of how we implement this clearing of revisions, we might be able to use cache tags for this, now that we have a proper cache backend for the static cache. I.e. we could tag the revision cache entries with the entity's cache tag(s) and get the invalidation for free. We need to make sure, though, that that actually works as we expect.
• Related to #112 and explicitly mentioned in #113 I think it would make sense to have resetRevisionCache() take an array of revision IDs instead of entity IDs. @hchonov agreed with this. This is relevant to the dicussion above, because, even if we only clear the default revision in resetCache($old_revision->id()) we could then tell people if they want non-default revisions to be cleared they can call resetRevisionCache($old_revision_id->getRevisionId())

While starting with the re-implementation, some more questions came up, so this is another "review" of the current patch.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -35,6 +42,19 @@
   +  /**
   +   * Whether the static revision cache should be ignored when loading an entity.
   +   *
   +   * This property will be set when loading the unchanged entity or entity
   +   * revision in order for ::load() and ::loadRevision() to load the entity
   +   * without using the static entity cache or static entity revision cache.
   +   * @see ::loadUnchanged()
   +   * @see ::loadRevisionUnchanged()
   +   *
   +   * @var bool
   +   */
   +  protected $ignoreStaticRevisionCache = FALSE;
   

   So I guess this is because ::loadUnchanged() actually doesn't just ignore the static cache, but clears it. So with

   $a = $storage->load(5);
   $storage->loadUnchanged(5);
   $b = $storage->load(5);
   

   $a === $b will yield FALSE.

   So by using this flag and not actually clearing the static cache you can achieve a situation where with

   $a = $storage->loadRevision(5)
   $storage->loadRevisionUnchanged();
   $b = $storage->loadRevision(5)
   

   $a === $b will yield TRUE. That is nice, but

   1. we should document it more explicitly, in particular the difference between loadUnchanged() and loadRevisionUnchanged()
   2. I wonder whether the difference between the two is actually confusing enough that it is warranted to make loadRevisionUnchanged() also clear the static cache. Even though it would be a bit unfortunate, perhaps the consistency outweighs the idempotency? Not sure.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -446,6 +466,41 @@ public function purgeFieldData(FieldDefinitionInterface $field_definition, $batc
   +  public function load($id) {
   

   Since loadMultiple() is not being overidden here, I think this "fallback to the revision cache" only happens when loading a single entity, not multiple ones. That is a bit unfortunate, if true. I think if we implement #109.2 we can remove this entire method override and then also get rid of this inconsistency.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -991,22 +1196,194 @@ public function loadUnchanged($id) {
   +  protected function getFromStaticCache(array $ids, $revision = FALSE) {
   ...
   +  protected function setStaticCache(array $entities, $revision = FALSE) {
   

   While this goes again #109.14 I just now realized that we are extending the base methods and introducing an additional parameter. Since we end up doing the if-else in the method, anyway, I wonder whether we shouldn't introduce separate getFromStaticRevisionCache() and setStaticRevisionCache() methods.

OK, opened #3013917: Minor improvements for (Content|)EntityStorageBase for #109.3 + #109.4

OK, here's a complete rework of the patch done in collaboration with @hchonov and me. Done with literal pair programming and then also internal patch-ping-pong until we were both at a point where we are confident with the patch.

For now not providing an in-depth explaining of the decisions here, will add that to the issue summary, so we can keep that updated if we refine the implementation here.

First, I want to see what kind of test failures we have now.

Here's an interdiff for the coding standards fixes.

It seems to me that we're trying to do too many things in this issue. Since the static cache seems to be the most problematic part, how about splitting up the work and create a separate issue for enabling persistent caching of revisions, since that brings the most performance benefits anyway?

Tbh, I think the consistency part is only an artificial problem, if we can call it a problem at all. Are there any examples (code or even conceptual) where an identical comparison check between entity revision objects is actually useful or can not be accomplished in another way?

Because the other side of the coin (the performance aspect) offers real and measurable benefit :)

@hchonov, I really can't tell if you're being serious or not. I know what static caching does.

If this is your problem with the static cache, then I have good news :). It doesn't only ensure consistency, but also brings performance gain, which also "offers real and measurable benefit :)".

I don't have any problem with the static cache, the two sides of the coin were performance and consistency.

Call 100 times $storage->loadRevision($revision_id with enabled static cache and the entity storage will have to retrieve the entity from the persistent cache only once.
Call 100 times $storage->loadRevision($revision_id) without enabled static cache and the entity storage will have to retrieve the entity from the persistent cache not only once, but 100 times.

That's super helpful to know! Thanks!

Please read #126 again, all I'm saying is that it would be easier if we add persistent caching for revisions first, which doesn't have any questionable things like #122 and #125 to address, and then work on enabling static cache as well and discuss those issues in depth.

Just a re-roll of #125

I just finished reading the entire issue and skimmed every linked issue that is not yet closed.

Thanks for the many interesting issues that have been linked from here, for the multiple separate issues that have already landed that cleaned up some areas, and for the thoughtful discussion here! Especially kudos to @hchonov and @tstoeckler for their meticulous work. 👏👏👏

It seems that the importance of this issue has only grown since this was opened, like @catch pointed out in #20 about two and a half years ago. I think that's even more true today.

#133's reroll is failing on a test that has existed since April 2018, so something in the foundations has probably changed that's causing that failure — it's not a new test, it's one that was green in #125.

I understand and sympathize with #132 in that it's indeed easier to develop two related things in a single patch. Sometimes it's absolutely necessary. I don't think it is absolutely necessary in this case. A 50K patch against Entity API is hard to land. I think you'll find that by extracting the static caching from this patch and leaving that as a second step (with the code already fully done!) you'll get to commits (and hence progress) faster 🙂 Actually, it may even be informative to just split up the patch here in a "phase 1: persistent revision caching" patch and a "phase 2; static revision caching" patch plus a combined patch. That alone will make this easier to review, because it'll be clearer how the two relate to each other. I'm sure it's already clear to those who spent many (MANY!) hours writing this patch, but it's hard for reviewers to gain that same level of understanding.
So I propose that as a first step, we split this patch up in two *-do-not-test.patch patches just to make reviews easier. Let's then check again whether we believe splitting this up in two issues is worth it.

Rebased #133, it conflicted with #3025427: Add @trigger_error() to deprecated EntityManager->EntityTypeBundleInfo methods. Also fixed trivial nits (see interdiff).

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -668,6 +769,38 @@ protected function doPreSave(EntityInterface $entity) {
   +    // Tentatively set the flag for ::resetCache() to skip invalidating all
   

   🤔 Why tentatively? Code should be doing things with certainty, not tentatively. That word frightens me, especially deep in Entity API :)

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -963,11 +1097,15 @@ protected function cleanIds(array $ids, $entity_key = 'id') {
   +   * @param bool $revision
   +   *   (optional) Defines whether the passed $ids are entity IDs or revision
   

   🤔 Wouldn't $are_revision_ids be a clearer name?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -1001,13 +1139,44 @@ protected function setPersistentCache($entities) {
   +          // When the default revision is cleared from the revision cache via
   +          // ::resetRevisionCache() we must clear the respective entity from
   +          // the entity cache, as well. To do this, we add a tag with the
   +          // revision ID of the default revision to the entity's cache entry.
   +          // This tag will be invalidated by ::resetRevisionCache().
   +          // @see \Drupal\Core\Entity\ContentEntityStorageBase::resetRevisionCache()
   +          $entity_cache_tags[] = "default_revision:{$this->entityTypeId}:{$entity->getRevisionId()}";
   

   🤔 Per-revision cache tags will lead to an exponential increase in the number of cache tags in existence for some sites. The cachetags DB table will grow enormous.
   Is this absolutely necessary? Why?

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -1059,18 +1228,141 @@ public function loadUnchanged($id) {
   +  protected function buildCacheId($id, $revision = FALSE) {
   

   🤔 $is_revision would be clearer?

5. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -276,12 +276,30 @@ public function loadMultiple(array $ids = NULL) {
   +    // TODO when the revision cache is enabled and an entity is loaded in its
   +    // default revision through ::loadRevision(), then the entity will be added
   +    // both to the entity and entity revision cache. In order to make it
   +    // possible to exchange an entity with preLoad if it is loaded by ID later,
   +    // the preLoad has to occur before we retrieve the entity from the static
   +    // cache. In order to prevent the multiple execution of the preLoad hooks we
   +    // could create additional static cache entries for preLoaded entities, for
   +    // example by adding a prefix or suffix "-preLoaded" to the cache ID.
   

   🤔 From reading #122, I thought this work had already been done. Apparently it still needs to happen?

6. +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   @@ -446,6 +469,7 @@ public function save(EntityInterface $entity) {
        // Perform the save and reset the static cache for the changed entity.
        $return = $this->doSave($id, $entity);
   +    $this->doResetCacheOnSave($entity);
   
   @@ -516,8 +553,6 @@ protected function doPreSave(EntityInterface $entity) {
      protected function doPostSave(EntityInterface $entity, $update) {
   -    $this->resetCache([$entity->id()]);
   -
        // The entity is no longer new.
   

   🤔 Why do we need to change when the cache is reset? How confident are we that this won't cause behavior changes?

7. +++ b/core/modules/node/tests/src/Functional/NodeRevisionsAllTest.php
   @@ -186,12 +186,9 @@ public function testRevisions() {
   -    $connection->update('node_revision')
   -      ->condition('vid', $nodes[2]->getRevisionId())
   -      ->fields([
   -        'revision_timestamp' => $old_revision_date,
   -      ])
   -      ->execute();
   +    node_revision_load($nodes[2]->getRevisionId())
   +      ->setRevisionCreationTime($old_revision_date)
   +      ->save();
   
   +++ b/core/modules/node/tests/src/Functional/NodeRevisionsTest.php
   @@ -192,12 +192,9 @@ public function testRevisions() {
   -    $connection->update('node_revision')
   -      ->condition('vid', $nodes[2]->getRevisionId())
   -      ->fields([
   -        'revision_timestamp' => $old_revision_date,
   -      ])
   -      ->execute();
   +    node_revision_load($nodes[2]->getRevisionId())
   +      ->setRevisionCreationTime($old_revision_date)
   +      ->save();
   

   🤔 Why are these changes necessary?

   I think: Because direct DB queries don't trigger the necessary invalidation. This should be listed in the issue summary as a sort-of-API-change (I know it isn't really, but if core was still doing this until now, then probably some contrib/custom modules are too). It definitely needs a change record of its own.

   Tagging Needs change record for this.

8. 🥳 Finally, I think that you could massively boost interest in this patch (and excitement about it) by turning #129's analysis into concrete numbers: what are the performance gains on the project you're using this on, what are the performance gains on a site that has Workspaces turned on, or Content Moderation? Ideally, we'd find a real-world deployment of either of those two that is suffering from performance problems and this patch makes a huge difference 😀

   Tagging Needs profiling for this.

Straight reroll against 8.8.x, lets see if any more tests fail.

Fortunately (well, unfortunately for me!) I've got that real-world performance problem.

We're replicating an existing API that returns a response that is both wide (many entities) and deep (many entity references within paragraph fields). Our serializer calls referencedEntities on paragraph fields, which means that loadRevision is called. On one request we're calling loadRevision ~340 times.

The total response times can be ignored - this is with other caches disabled and other unrelated performance fixes to make.

The static cache alone saves around 340ms.

A second load with a primed persistent cache saves an additional 1080ms or so:

Some ab results without xhgui (as that affects absolute numbers significantly)

Before this patch:

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       0
Processing:  7166 7502 445.3   7381    8281
Waiting:     7163 7499 444.9   7377    8278
Total:       7166 7503 445.4   7381    8282

After:

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    1   0.9      1       2
Processing:  6451 6708 318.0   6674    7245
Waiting:     6447 6705 317.7   6671    7242
Total:       6451 6708 317.9   6675    7245

The test failure is triggered in \Drupal\Tests\workspaces\Kernel\WorkspaceIntegrationTest::assertWorkspaceStatus when it's switching the workspace and checking the home page view. That's as far as I've got - I've not used Workspaces at all so it will take some time to debug.

> Our serializer calls referencedEntities on paragraph field

Don't do that. We implemented an optimization in ERR that assumes that the referenced paragraph revision is the default revision, which is typically the case when loading the default revision of the host entity. Only if that doesn't match we try to load by revision. At least until this issue is resolved.

However, we forgot to implement that for referencedEntities() too, so instead, just do foreach ($items as $item) and access ->entity.

Thanks! I'll take a look at that.

I haven't found the root cause of the failure, but I did find a side effect that may help in tracking it down. In \Drupal\Tests\workspaces\Kernel\WorkspaceIntegrationTest::assertWorkspaceStatus(), if we call loadUnchanged() before rendering the view, the test passes. Remove that call and we're back to failing.

Reroll of patch from #138 against 8.9.x.

In this patch update, preLoad implementing hook_entity_preload() was moved before static cache when loading an entity, to allow this hook implementations still to act where there is already a default revision loaded. It was actually a todo in this patch. And separate cache for preload hook results was introduced. And the latter is kept separate from "normal" entities static cache. The patch also contains a test for that.
There was another test failure in content moderation. The assertion that failed was introduced in this patch, and i can't see why it should be met. Once reverted ContentModerationStateTest.php to the head 8.9 branch, test passes.

Minor updates to the test.

Another try at fixing all tests. This time that's because loadUnchanged() doesn't check its argument. Created a separate issue for that #3099709: Prevent null parameter value in loadUnchanged() like in load()

Seems that !empty() was too risky condition. Changing to !==NULL.

We encountered the problem showing a view with node revision data.
The count of SQL-Statements skyrocketed to more than 5000 and the page got so slow it exceeded PHP's max_execution_time of 240 seconds.

My colleague um adapted the patch in #151 so we could apply it to 9.3.5.
Seems to work for us, but we did not perform much testing yet - so we will upload the patch for 9.3.5 after we did a little more testing.