Problem/Motivation

#597236: Add entity caching to core added the entity cache to core, but unfortunately it did not consider enabling it when loading an entity by revision ID.
There are contrib modules such as Entity Reference Revisions, Paragraphs and core modules such as Content Moderation, which rely heavily on loading entity revisions.

A developer might build a system where she is using ContentEntityStorageBase::loadRevision() to load a default entity revision and by doing so one will currently not profit from the entity cache.

ContentEntityStorageBase::loadRevision() is neither using the static entity cache nor the persistent entity cache, which has as a consequence that the function is inconsistent with ContentEntityStorageBase::load(), as if you retrieve an entity multiple times in a single request with the latter one it will always return a reference to the same entity object, but if you retrieve it multiple times with the former one you will always get a new entity object and thus a completely different object reference.

The attached test demonstrates all the problems.

Proposed resolution

  1. Enable static entity caching for ContentEntityStorageBase::loadRevision() for all revisions.
  2. Enable persistent entity caching for ContentEntityStorageBase::loadRevision() for all revisions.
  3. Ensure that ContentEntityStorageBase::load() and ContentEntityStorageBase::loadRevision() return a reference to the same entity object, if loading the default entity revision.
  4. To ensure backwards compatibility $storage::resetCache() will be invalidating all cached revisions. Include an optimization to invalidate only the default revision when the entity has only revisionable fields.

Remaining tasks

Review.

User interface changes

none

API changes

  1. \Drupal\Core\Entity\RevisionableStorageInterface::loadRevisionUnchanged() to load an entity revision bypassing the static cache.
  2. \Drupal\Core\Entity\RevisionableStorageInterface::resetRevisionCache($revision_id) for reseting the static and persistent cache for specific revision IDs
  3. $storage->loadRevision($revision_id) === $storage->loadRevision($revision_id)now evaluates to TRUE
  4. $storage->load($id) === $storage->loadRevision($revision_id)now evaluates to TRUE if $revision_id references the current default revision

Data model changes

none

CommentFileSizeAuthor
#172 reroll_diff_168-172.txt9.89 KBmuriqui
#172 2620980-172.patch54.31 KBmuriqui
#171 2620980-nr-bot.txt145 bytesneeds-review-queue-bot
#168 interdiff-2620980-166-168.txt455 bytesyogeshmpawar
#168 2620980-168.patch54.26 KByogeshmpawar
#166 interdiff-2620980-164-166.txt747 bytesyogeshmpawar
#166 2620980-166.patch54.12 KByogeshmpawar
#164 interdiff-2620980-162-164.txt1.63 KByogeshmpawar
#164 2620980-164.patch54.05 KByogeshmpawar
#162 interdiff-2620980-158-162.txt1.76 KByogeshmpawar
#162 2620980-162.patch54.12 KByogeshmpawar
#158 core--revision-cache-2620980-158-9.3.5.patch54.09 KBgngn
#151 interdiff-149-151.txt490 bytesgease
#151 2620980-151.patch53.77 KBgease
#149 interdiff-146-149.txt885 bytesgease
#149 2620980-149.patch53.77 KBgease
#146 interdiff-145-146.txt2.41 KBgease
#146 2620980-146.patch53.39 KBgease
#145 interdiff-144-145.txt11.13 KBgease
#145 2620980-145.patch53.48 KBgease
#144 2620980-144.patch47.79 KBgease
#141 2620980-141-show-sideeffect.patch48.8 KBdeviantintegral
#141 2620980-141-interdiff-show-sideeffect.txt1.07 KBdeviantintegral
#139 plus-persistent-cache.png227.02 KBdeviantintegral
#139 static-cache.png228.04 KBdeviantintegral
#138 2620980-136.patch47.73 KBdeviantintegral
#135 2620980-135.patch47.73 KBWim Leers
#135 interdiff.txt5.18 KBWim Leers
#133 2620980-133.patch47.79 KBchr.fritsch
#125 2620980-125.patch49.89 KBhchonov
#125 interdiff-122-125.txt3.69 KBhchonov
#122 2620980-122.patch46.89 KBhchonov
#122 interdiff-119-122.txt3.79 KBhchonov
#121 2620980-119-interdiff-cs.txt1.48 KBtstoeckler
#119 2620980-119.patch45.8 KBhchonov
#119 interdiff-117-119.txt3.03 KBhchonov
#117 2620980-117.patch43.91 KBtstoeckler
#108 interdiff-106-108.txt5.25 KBhchonov
#108 2620980-108.patch56.81 KBhchonov
#106 interdiff-105-106.txt8.29 KBhchonov
#106 2620980-106.patch53.17 KBhchonov
#105 interdiff-103-105.txt5.07 KBhchonov
#105 2620980-105.patch49.89 KBhchonov
#103 interdiff-100-103.txt3.71 KBhchonov
#103 2620980-103.patch48.29 KBhchonov
#100 interdiff-99-100.txt2.64 KBhchonov
#100 2620980-100.patch43.72 KBhchonov
#99 interdiff-97-99.txt9.3 KBhchonov
#99 2620980-99.patch41.59 KBhchonov
#97 interdiff-95-97.txt3.83 KBhchonov
#97 2620980-97.patch39.05 KBhchonov
#95 2620980-95.patch37.62 KBhchonov
#93 2620980-92.patch33.17 KBkfritsche
#90 2620980-90.patch34.89 KBSam152
#90 reroll.txt3.06 KBSam152
#86 interdiff-84-86.txt5.77 KBhchonov
#86 2620980-86.patch33.5 KBhchonov
#86 2620980-86-test-only.patch6.88 KBhchonov
#84 interdiff-80-84.txt5.22 KBhchonov
#84 2620980-84.patch28.72 KBhchonov
#80 interdiff-78-80.txt4.16 KBhchonov
#80 2620980-80.patch29.6 KBhchonov
#78 interdiff-75-78.txt3.87 KBhchonov
#78 2620980-78.patch27.82 KBhchonov
#75 2620980-75.patch23.77 KBtstoeckler
#70 interdiff-67-70.txt3.96 KBhchonov
#70 2620980-70.patch23.85 KBhchonov
#67 interdiff-64_rerolled-67.txt6.1 KBhchonov
#67 2620980-67.patch19.13 KBhchonov
#67 2620980-64-rerolled.patch19.94 KBhchonov
#64 2620980-64.patch19.93 KBhchonov
#63 interdiff-62-63.txt671 byteshchonov
#63 2620980-63.patch19.96 KBhchonov
#62 interdiff-61-62.txt3.77 KBhchonov
#62 2620980-62.patch19.99 KBhchonov
#61 2620980-61.patch19.91 KBhchonov
#59 2620980-59.patch18.61 KBamateescu
#52 2620980-52.patch25.28 KBtimmillwood
#48 2620980-48.patch28.58 KBtimmillwood
#45 interdiff.txt3.27 KBtimmillwood
#45 2620980-45.patch28.58 KBtimmillwood
#35 interdiff.txt2.19 KBtimmillwood
#35 2620980-35.patch29.25 KBtimmillwood
#27 interdiff-23-27.txt960 byteshchonov
#27 2620980-27.patch27.72 KBhchonov
#23 2620980-23.patch26.65 KBcspitzlay
#22 interdiff-2620980-19-22.txt0 bytescspitzlay
#21 interdiff-2620980-19-21.txt751 bytescspitzlay
#21 2620980-21.patch26.21 KBcspitzlay
#19 interdiff-17-19.txt1.12 KBhchonov
#19 2620980-19.patch26.6 KBhchonov
#17 2620980-17.patch25.31 KBhchonov
#14 interdiff-12-14.txt1.32 KBhchonov
#14 2620980-14.patch25.33 KBhchonov
#12 2620980-12.patch25.17 KBhchonov
#12 10-12-interdiff.txt2.46 KBhchonov
#10 2620980-10.patch24.24 KBhchonov
#10 8-10-interdiff.txt4.85 KBhchonov
#8 2620980-8.patch21 KBhchonov
#8 6-8-interdiff.txt5.93 KBhchonov
#6 4-6-interdiff.txt5.09 KBhchonov
#6 2620980-6.patch21.12 KBhchonov
#4 2620980-4.patch20.01 KBhchonov
load_revision_inconsistent_with_load_regarding_entity_cache.patch3.53 KBhchonov

Issue fork drupal-2620980

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

I am currently working on a possible patch.

Status: Needs review » Needs work
hchonov’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
20.01 KB

First implementation....

Status: Needs review » Needs work

The last submitted patch, 4: 2620980-4.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
21.12 KB
5.09 KB

Status: Needs review » Needs work

The last submitted patch, 6: 2620980-6.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
5.93 KB
21 KB

Status: Needs review » Needs work

The last submitted patch, 8: 2620980-8.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
4.85 KB
24.24 KB

Status: Needs review » Needs work

The last submitted patch, 10: 2620980-10.patch, failed testing.

hchonov’s picture

hchonov’s picture

Assigned: hchonov » Unassigned
hchonov’s picture

If loading a default revision by revision id it should be added to the static entity cache as well.

hchonov’s picture

Issue tags: +consistency

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hchonov’s picture

Rerolling for 8.1.x.. Nothing changed between this and #14.

Status: Needs review » Needs work

The last submitted patch, 17: 2620980-17.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
26.6 KB
1.12 KB

As now the function loadRevision is moved to the ContentEntityStorageInterface we have to use this inteface when writing tests and mockups for it.

catch’s picture

+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.

cspitzlay’s picture

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

cspitzlay’s picture

FileSize
0 bytes

reroll of #19

cspitzlay’s picture

reroll of #19

The last submitted patch, 21: 2620980-21.patch, failed testing.

cspitzlay’s picture

Status: Needs review » Needs work

The last submitted patch, 23: 2620980-23.patch, failed testing.

hchonov’s picture

Version: 8.1.x-dev » 8.3.x-dev
Status: Needs work » Needs review
FileSize
27.72 KB
960 bytes

Status: Needs review » Needs work

The last submitted patch, 27: 2620980-27.patch, failed testing.

The last submitted patch, 27: 2620980-27.patch, failed testing.

The last submitted patch, 27: 2620980-27.patch, failed testing.

jibran’s picture

Title: ContentEntityStorageBase::loadRevision should be consistent with ContentEntityStorageBase::load and use the static and the persistent entity cache » ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load()

Should we profile this as well?

hchonov’s picture

Not really sure that this needs profiling as it addresses mostly inconsistencies :

  • calling ::loadRevision() multiple times for the same revision id returns always a new entity object
  • calling ::loadRevision() for the default revision will return a different object than ::load()

So the changes here are needed and not necessary done for performance but mostly for consistency reasons.

timmillwood’s picture

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.

timmillwood’s picture

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.

timmillwood’s picture

This should fix it.

Status: Needs review » Needs work

The last submitted patch, 35: 2620980-35.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
hchonov’s picture

+++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
@@ -159,6 +159,7 @@ public function save() {
+    $this->entityTypeManager()->getStorage('content_moderation_state')->resetCache([$this->id()]);
     return $related_entity->save();

You do not need to reset the cache here. You could only exchange the entity :

$related_entity->moderation_state->entity = $this;
timmillwood’s picture

@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

timmillwood’s picture

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.

hchonov’s picture

Ok I now understand what you mean but I do not understand really your previous statement:

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.

Whenever you load an entity in the system it should be a reference to the same entity object no matter if you load by id or reference. That said I do not get it how at one place you have the previously cached entity and at another one an other object. Do you think this might be a bug in the patch? Would you please clarify in more details what exactly happens?

timmillwood’s picture

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.

timmillwood’s picture

Version: 8.3.x-dev » 8.2.x-dev
Issue tags: +Workflow Initiative

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,

hchonov’s picture

Status: Needs review » Needs work

The patch still has some places, which need a rework.

  1. --- a/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
    +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
    
    +++ b/core/lib/Drupal/Core/Entity/EntityStorageInterface.php
    @@ -72,27 +72,6 @@ public function load($id);
    -  public function loadRevision($revision_id);
    ...
    -  public function deleteRevision($revision_id);
    

    We could not remove the revision functions from the EntityStorageInterface until D9.

  2. diff --git a/core/lib/Drupal/Core/Entity/RevisionableInterface.php b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
    
    +++ b/core/lib/Drupal/Core/Entity/RevisionableInterface.php
    @@ -40,6 +40,29 @@ public function setNewRevision($value = TRUE);
    +  public function getOriginalRevisionId();
    ...
    +  public function resetOriginalRevisionId();
    

    These two functions should be put into a new interface, otherwise this represents an API break.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
28.58 KB
3.27 KB

Addressed items from #44

Status: Needs review » Needs work

The last submitted patch, 45: 2620980-45.patch, failed testing.

The last submitted patch, 45: 2620980-45.patch, failed testing.

timmillwood’s picture

Version: 8.2.x-dev » 8.3.x-dev
FileSize
28.58 KB

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.

timmillwood’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 48: 2620980-48.patch, failed testing.

The last submitted patch, 48: 2620980-48.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
25.28 KB

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

amateescu’s picture

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.

timmillwood’s picture

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?

Berdir’s picture

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?

hchonov’s picture

Addressing #53:

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])?

If we do this then when the method ::loadRevision($revision_id) is called we do not now at this point to which entity id the revision id is assigned, sure we could iterate over the array and search for it - not sure what is better.

Addressing #54:
I would say that here we already have something working and test coverage for it, so as soon as the nitpicks are cleared it should be ready to go and later the other issue for exchanging the cache backend will need a reroll to implement the missing functionality. We have a problem here and the sooner we fix it the better.

Addressing #55:

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.

Sure, we could first check if the revision entity key is present, but this adds just more code that is not necessary needed as getRevisionId will make this check as well and if the key is not present it will just return NULL.

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?

Yes, you understand the intention correctly and yes I think the comment could need a refactoring :).

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?

If we do it this way then for the default revision we would need two entries in the entity cache array -
[entity-id]['default'] and [entity-id]['revision_id'] so that loadRevision could search for the revision id in the entity cache array.

catch’s picture

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.

timmillwood’s picture

Status: Needs review » Postponed

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.

amateescu’s picture

FileSize
18.61 KB

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

timmillwood’s picture

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.

hchonov’s picture

FileSize
19.91 KB

Updating the patch based on the changes in #2809227: Store revision id in originalRevisionId property to use after revision id is updated and I've made also a little bit of code refactoring.

hchonov’s picture

FileSize
19.99 KB
3.77 KB

In #61 I've started optimizing loadRevisionUnchanged but forgot to finish my work .. oups.

hchonov’s picture

FileSize
19.96 KB
671 bytes

In doLoadRevision only write to the persistent cache if we are loading a default revision.

hchonov’s picture

Status: Postponed » Needs review
FileSize
19.93 KB

#2809227: Store revision id in originalRevisionId property to use after revision id is updated got committed so this is issue is not postponed anymore on it. I've re-rolled the patch.

jibran’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 64: 2620980-64.patch, failed testing.

hchonov’s picture

#65
2., 6., 8. & 11. Done

1. The todo is about determining the revision id based on the enitity id, so I do not understand how #2809227: Store revision id in originalRevisionId property to use after revision id is updated is solving this?
3. There is no need of restoring it, as it is only set by loadRevisionUnchanged and read by loadRevision. I've actually forgotten to introduce the property to the class, so I've added it now.
4.1 Well content entities use integer for their ids so why should it be string|int instead of only int?
4.2 I've kept this identical to EntityStorageInterface and actually like said in 4.1 content entities use integers for their ids, so if changing anything maybe allowing only integers?
5. The doc is now defining the param of the revision id as being int.
7. Like in 1. I do not see how #2809227: Store revision id in originalRevisionId property to use after revision id is updated should be of any help here?
9., 10. I've added the same todo to the new methods now as the one from KeyValueContentEntityStorage::createTranslation :

// @todo Complete the content entity storage implementation in
// https://www.drupal.org/node/2618436.

I've also removed the changes to the content_moderation module, as there are new fails again and I am unable of understanding what happens there.

The last submitted patch, 67: 2620980-64-rerolled.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 67: 2620980-67.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
23.85 KB
3.96 KB

Fixing the content_moderation test fails.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tstoeckler’s picture

Status: Needs review » Needs work
  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?

tstoeckler’s picture

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.

tstoeckler’s picture

Issue tags: +Needs tests

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...

tstoeckler’s picture

hchonov’s picture

#72.1

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.

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.

#72.2

What is this?

I guess this is a some left over from something I was experimenting with, so we can just remove it.

#73

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.

This wasn't earlier the case. We previously cleared the cache by the CESB::resetCache implementation. This changed in #2753675: loadUnchanged should use the persistent cache to load the unchanged entity if possible instead of invalidating it and I've forgotten to update the patch in this issue here. So yes, we have to clear the static revision cache additionally. Good catch :).

tstoeckler’s picture

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;?

hchonov’s picture

I was able to reproduce the problem and it would happen if the entity is statically and persistently cacheable and also revisionable. The use case is as follow:

$entity = $storage->create();
$entity->save();

$entity = $storage->load($entity->id());
// Do something to invalidate the entity cache e.g. create a field config and save it in an entity insert hook.
$unchanged = $storage->loadUnchanged($entity->id);

What happens now is that loadUnchanged will return the same reference to the entity object because the static entity cache has not been reset properly.

The attached patch has the test for this and will fail.

Berdir’s picture

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.

hchonov’s picture

Fixing the failing test provided in #78.

The last submitted patch, 78: 2620980-78.patch, failed testing.

timmillwood’s picture

Issue tags: -Needs tests

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

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

vijaycs85’s picture

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.

hchonov’s picture

#83:
1. Done.
2. Nice catch.. Actually currently in HEAD we execute the post load only in the loading functions, not in the ones retrieving from the persistent cache, therefore we should remove that piece of code from the patch from getRevisionFromPersistentCache. Done.
3. Done.
4. Done.
5. I've redefined load, loadRevision and deleteRevision in ContentEntityStorageInterface in order for them to have an annotation that they are returning content entities, but this is not necessary change for the current patch -> reverting that change.
6. Done.

Thank you, @vijaycs85!

vijaycs85’s picture

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?

hchonov’s picture

@vijaycs85, I've extended the already provided ContentEntityStaticCacheTest in the patch. As it tests also the new method loadRevisionUnchanged I had to remove that piece of testing the new method in order to provide a test only patch, but of course I've kept it in the whole patch :).

The last submitted patch, 86: 2620980-86-test-only.patch, failed testing.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Status: Needs review » Needs work

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.

Sam152’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
34.89 KB

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

hchonov’s picture

Unfortunately the current patch is not considering non-revisionable fields and they will be cached together with the entity, but we don't have any way yet of loading their current values. I can think of three ways for solving this:

  1. In \Drupal\Core\Entity\Sql\SqlContentEntityStorage::saveToDedicatedTables() always call ::hasFieldValueChanged() for non-revisionable fields and if they've changed invalidate the revision cache tag for that entity - still have to introduce the revision cache tag.
  2. On loading a revision from the persistent entity cache then load the default revision as well and copy over the values of the non-revisionable fields.
  3. On loading a revision from the persistent entity cache then load the field values of the non-revisionable fields through an query.
  4. On saving always persistently cache the values of non-revisionable fields per entity ID and on loading an entity from the persistent revision cache copy over the persistently cached non-revisionable field values, which in comparison to the previous option will spare us the work of building a query for multiple fields and we will retrieve the values really easy.

Any opinions on which option is or might be the better choice?

Sam152’s picture

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

kfritsche’s picture

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hchonov’s picture

This is a re-roll and at the same time adaptation to the new storage logic as a lot of things have changed in the storage in the meanwhile and no straightforward re-roll was possible. I've tried to further simplify the patch and reuse as much as possible from the provided infrastructure.

This patch still doesn't fix #91.

Status: Needs review » Needs work

The last submitted patch, 95: 2620980-95.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

Status: Needs work » Needs review
FileSize
39.05 KB
3.83 KB

One of the test fails was caused by an endless loop triggered by the workspace entity query class, where if during entity load an entity query is executed then in the preparation phase another entity load will occur leading to the endless loop. I am not sure how we can deal with such a problem, but for now I've flagged the query with a tag, which is checked in the workspace entity query class to prevent loading the entity again.

amateescu’s picture

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 :)

hchonov’s picture

@amateescu, it is nice, that there is already an issue about that :). I think I am gonna stick for now to the small fix I have here as otherwise the patch will get much bigger ... I guess I'll have to deal with the removed code later :).

So I was trying to develop a test for #91, but it wasn't failing. Then I've realized that the clearing of the revision caches wasn't optimal and this is why the revision caches have been always deleted on entity save... I've optimized the patch now and the test is failing now as we would've expect.

hchonov’s picture

And here is solution for that problem.. It is not nice but we would have to wait for #2862574: Add ability to track an entity object's dirty fields (and see if it has changed) to get a nice solution.

The last submitted patch, 99: 2620980-99.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 100: 2620980-100.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

Status: Needs work » Needs review
FileSize
48.29 KB
3.71 KB

I was struggling a lot with the latest test failures, until I've asked @tstoeckler for help and he found the reason for the test failures very quickly. We think that the problem existed even before the patch, but we don't why it hasn't occurred before. What happens is that a duplicate of an entity with a path alias is created and the path alias is not unset on the duplicate leading to two entries of the same alias pointing to two different entities. Later on an incoming request for that alias with the patch applied the duplicate entity was picked instead of the first one.

@tstoeckler++

Status: Needs review » Needs work

The last submitted patch, 103: 2620980-103.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

Status: Needs work » Needs review
FileSize
49.89 KB
5.07 KB

Here is a fix for the last test failures... I had to add a clean up of the persistent revision cache on entity delete and entity revision delete.

hchonov’s picture

I was working on additional test coverage for deleting entities and entity revisions and discovered some more problems with the patch, which I've fixed in the new version:

- The entity revision ID field is not flagged as revisionable and therefore it has to be excluded explicitly of the check for changes in non-revisionable fields.
- During the re-rolls somehow I've forgot the piece of code updating the default revision status.
- In some comments I was using "ids" instead of "IDs".

Status: Needs review » Needs work

The last submitted patch, 106: 2620980-106.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

The test failures are showing that the patch is starting to function properly :).

We have places in the tests where field values are altered directly in the revision table through the database API and the entities are loaded by revision IDs. As the revisions weren't cached previously this was fine, but now the revisions are cached persistently and therefore the persistent revision cache has to be cleared after such an action.

tstoeckler’s picture

Status: Needs review » Needs work

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.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tstoeckler’s picture

  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 (+=).

tstoeckler’s picture

+++ 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()

tstoeckler’s picture

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.

tstoeckler’s picture

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())
tstoeckler’s picture

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.

tstoeckler’s picture

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Needs change notice
FileSize
43.91 KB

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.

Status: Needs review » Needs work

The last submitted patch, 117: 2620980-117.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hchonov’s picture

Status: Needs work » Needs review
FileSize
3.03 KB
45.8 KB

We had some unfinished work regarding the cache tags, which seems to fix some of the test fails.

The changes in SqlContentEntityStorageTest are required as it is mocking a content entity only through the EntityInterface, but now in ContentEntityBase:setPersistentCache() we call ::isDefaultRevision() on the entity, but that method isn't present on the EntityInterface.

Status: Needs review » Needs work

The last submitted patch, 119: 2620980-119.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tstoeckler’s picture

FileSize
1.48 KB

Here's an interdiff for the coding standards fixes.

hchonov’s picture

Status: Needs work » Needs review
FileSize
3.79 KB
46.89 KB

The problem with the workspaces tests was caused because of the preLoad hooks. With our revision cache, if an entity is loaded by revision ID in its default revision, then that entity will be put both in the entity and entity revision cache. In this case the preLoad will not occur, when loading the entity by ID because the entity already exists in the static entity cache. I and @tstoeckler think, that the idea for having the preLoad after the retrieval from the static cache was not to execute the preLoad hooks multiple times, but even if we put the preLoad before the retrieval from the static cache, we still could skip executing the preLoad hooks. For this we could create additional entries in the static cache for preLoaded entities and retrieve them from there before executing the preLoad hooks. The cache IDs of preLoaded entities should then be extended by a dedicated prefix or suffix, for example "preLoaded".

I've also applied the coding standards from #121 to the new patch.

Status: Needs review » Needs work

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

hchonov’s picture

An optimization contained in this patch for resetting the cache only on updates is causing the test fail of \Drupal\KernelTests\Core\Command\DbDumpTest::testScriptLoad() I've just created a dedicated issue for this optimization, where the test is also being adjusted - #3015374: Prevent unnecessary reseting of the entity cache for new entities on entity create/insert

P.S. it appears that the fails in Drupal\Tests\taxonomy\Kernel\Migrate\d7\MigrateTaxonomyTermTest are also caused by this optimization.

The next version of the patch for the current issue should therefore have that optimization removed.

hchonov’s picture

Status: Needs work » Needs review
FileSize
3.69 KB
49.89 KB

1. Removed the optimization for skipping the cache reset on entity insert.
2. Fixed the content moderation test fails by adjusting Content Moderation to have proper expectation when having revision cache and properly shared entity references.

The necessary changes in Content Moderation might be seen as an API break, but if looked at them as such, then this is also a necessary change because of the increased usage of loading an entity by revision ID introduced both by Content Moderation and Workspaces. Additionally all usages of Entity Reference Revisions / Paragraphs will also profit from the new revision cache.

amateescu’s picture

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?

hchonov’s picture

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?

I guess you are right, that this should simplify the patch a little bit.

My main reason for keeping this together until now was that the performance benefit is only one side of the coin. The other is consistency and as important as the performance gain.

What I mean is that currently we have
$storage->load($id) === $storage->load($id)
evaluating to TRUE, but having both
$entity = $storage->load($id); $entity === $storage->loadRevision($entity->getRevisionId())
and
$storage->loadRevision($revision_id) === $storage->loadRevision($revision_id)
evaluating to FALSE.

amateescu’s picture

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’s picture

Tbh, I think the consistency part is only an artificial problem, if we can call it a problem at all.

I am sorry, but one cannot simply define consistency as an "artificial problem". When talking about DX, then consistency is really important. If I as developer write code based on the assumption, that the storage is always returning the same entity reference when loading an entity by ID, then I would have the same expectation when loading an entity by revision ID.

Here is an example test case from core, when loading an entity by ID, but the very same use case is also valid when loading an entity by revision ID:

\Drupal\KernelTests\Core\Config\ConfigEntityStaticCacheTest::testCacheHit() {
    $storage = $this->container->get('entity_type.manager')
      ->getStorage($this->entityTypeId);
    $entity_1 = $storage->load($this->entityId);
    $entity_2 = $storage->load($this->entityId);
    // config_entity_static_cache_test_config_test_load() sets _loadStamp to a
    // random string. If they match, it means $entity_2 was retrieved from the
    // static cache rather than going through a separate load sequence.
    $this->assertSame($entity_1->_loadStamp, $entity_2->_loadStamp);
}

Like in the case of config_entity_static_cache_test_config_test_load() one could have implemented a hook_entity_load() to enhance the entity by additional information, which is retrieved from a slow storage, but it is valid only for the current request and therefore one could not use hook_entity_storage_load(), which will be caching the entity into the persistent cache.

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

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 :)".

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.

This is why I see the static and persistent cache of ::loadRevision() as one complete feature.

hchonov’s picture

Issue summary: View changes
hchonov’s picture

Issue summary: View changes
amateescu’s picture

@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.

chr.fritsch’s picture

Just a re-roll of #125

Status: Needs review » Needs work

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

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record, +needs profiling
FileSize
5.18 KB
47.73 KB

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.

Status: Needs review » Needs work

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

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

deviantintegral’s picture

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

deviantintegral’s picture

FileSize
228.04 KB
227.02 KB

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.

Static cache saves 340ms

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

Persistent cache saves 1080ms

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.

Berdir’s picture

> 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.

deviantintegral’s picture

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.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

apaderno’s picture

Issue tags: -Needs change notice
gease’s picture

Reroll of patch from #138 against 8.9.x.

gease’s picture

Status: Needs work » Needs review
FileSize
53.48 KB
11.13 KB

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.

gease’s picture

Minor updates to the test.

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

Status: Needs review » Needs work

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

gease’s picture

Status: Needs work » Needs review
FileSize
53.77 KB
885 bytes

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()

Status: Needs review » Needs work

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

gease’s picture

Status: Needs work » Needs review
FileSize
53.77 KB
490 bytes

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

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gngn’s picture

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.

hchonov’s picture

@gngn that's great! I was hoping also to be able soon to dedicate some time and get back to it... It would be great if you could post the reroll of the patch and then we can see what the automated tests tell. After having some time passed I also think like it was suggested here that we should divide the issue in two parts - one for the persistent and one for static cache, otherwise this issue will just get bigger and not have high chances of getting in. When you upload the patch and we fix the eventual test failures I would start working on dividing the approach.

gngn’s picture

Reroll for 9.3.5 and 9.3.6 (hope it applies to 9.4.x-dev).

We stumbled upon this issue trying to resolve a timeout issue with revisions.
The attached reroll of #135 seemed to help - but now we found out that we only reduced the number of timeouts - so we are still testing...

apaderno’s picture

Status: Needs review » Needs work
+    // Ensure that the preloaded entites are not influencing the default static
+    // entity cache.

It's preloaded entities.

gngn’s picture

@apaderno thanks for taking a look.

It's preloaded entities.

Do you mean the typo in the comment (preloaded entites should be preloaded entities) - or do you mean we are missing something about the preloaded entities?

apaderno’s picture

@gngn I meant the typo in the comment.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
54.12 KB
1.76 KB

Resolved CSpell errors in #158 & Addressed #159.

Status: Needs review » Needs work

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

yogeshmpawar’s picture

Updated patch will fix test failures.

Status: Needs review » Needs work

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

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
54.12 KB
747 bytes

Fix test failures & added interdiff.

apaderno’s picture

Status: Needs review » Needs work
yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
54.26 KB
455 bytes

Thanks @apaderno, Fixed CS issues & added an interdiff.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
145 bytes

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.

muriqui’s picture

Status: Needs work » Needs review
FileSize
54.31 KB
9.89 KB

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);

Status: Needs review » Needs work

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

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

weseze’s picture

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!

capysara made their first commit to this issue’s fork.

capysara’s picture

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.