Problem/Motivation

Revision loading is slow, there is no persistent or static cache and no API for multi-loading.

Proposed resolution

#2672206: EntityReferenceRevisionsFormatterBase::prepareView() is blindly copied and lies removes most of the prepareView() code because it doesn't make sense right now.

As an alternative, what we could consider is doing something similar to field_collection. Assume that the revision is the default revision, do a cached multi-load of all default revisions. Then check each, and if the revision ID differs, load that specific revision.

Remaining tasks

Needs to be specifically tested with TMGMT previewing of paragraphs.

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

jmuzz’s picture

Here is the field collection code the issue summary is talking about:

  public function getFieldCollectionItem($create = FALSE) {
    if (isset($this->entity)) {
      return $this->entity;
    }
    elseif (isset($this->target_id)) {
      // By default always load the default revision, so caches get used.
      $field_collection_item = FieldCollectionItem::load($this->target_id);
      if ($field_collection_item !== NULL && $field_collection_item->getRevisionId() != $this->revision_id) {
        // A non-default revision is a referenced, so load this one.
        $field_collection_item = \Drupal::entityTypeManager()->getStorage('field_collection_item')->loadRevision($this->revision_id);
      }
      return $field_collection_item;
    }
    ...

It's a method of the field type. ERR doesn't seem to have an equivalent method in its field type.

Where is the loading that the issue summary refers to?

jmuzz’s picture

Oh I get it, it's the fake loading that replaced the old loading code in the issue referenced in the summary.

The formatter shouldn't be responsible for loading the entities. Most typical formatters for the field type will need loaded entities, so it's better to provide a method for it in the field type rather then copying the loading code into each of them.

Berdir’s picture

formatters load entities, that's how it works in core (\Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase::prepareView), we can't change that.

jmuzz’s picture

Yeah but EntityReferenceFormatterBase doesn't have to do any special logic about loading the entity. It just makes a single function call for it. The overridden formatter should just make a single function call for it too it shouldn't be handling the revisions logic.

jmuzz’s picture

Status: Active » Needs review
FileSize
2.62 KB
miro_dietiker’s picture

Status: Needs review » Needs work
Related issues: +#2801321: New host revisions do not always create new composite entity revisions

See discussions at related issue.

+++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
@@ -202,6 +202,25 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
+  public function getReferencedEntity() {
+    if (isset($this->entity)) {
+      return $this->entity;
...
+      return $entity;

If this is the method to call, then it should maintain its cache.
Otherwise different callers will still result in multiple loads.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
1.58 KB

Good point.

Status: Needs review » Needs work

The last submitted patch, 8: interdiff-6-8.patch, failed testing.

The last submitted patch, 8: err-loading-2673076-8.patch, failed testing.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
1.59 KB

$this->entity is used by autocreate widgets that need to store an entity which hasn't been saved yet, so need to make a separate property for the desired cache.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/src/Plugin/Field/FieldFormatter/EntityReferenceRevisionsFormatterBase.php
    @@ -13,14 +13,18 @@ abstract class EntityReferenceRevisionsFormatterBase extends EntityReferenceForm
    +        elseif ($item->hasNewEntity()) {
               $item->_loaded = TRUE;
    

    not sure why we need the elseif here, why can't getReferencedEntity() return the new entity? doesn't it do that anyway as it returns $this->entity?

  2. +++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
    @@ -39,6 +39,11 @@ use Drupal\entity_reference_revisions\EntityNeedsSaveInterface;
       /**
    +   * Reference to the entity pointed to by this field.
    +   */
    +  protected $entity_cache;
    +
    
    @@ -202,6 +207,28 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
    +    elseif (isset($this->entity_cache)) {
    +      return $this->entity_cache;
    +    }
    

    missing @var, I like to initialize those explicitly to NULL to make it obvious that that's the default value.

    And then do an explicit !== NULL check. Prefer that over isset, which is IMHO not explicit enough for those.

    Also, I'd say that should use camel case.

    that said, I actually don't understand why we need two properties. That's very uncommon, ->entity is the managed defined property, coming up with something else is very strange, especialy since the return value of this is actually assigned to $item->entity again, so what's the point?

jmuzz’s picture

Thanks @berdir.

My understanding is that $this->entity is for when there is a new entity in the field and target_id can't be assigned because the entity doesn't have an ID yet. This happens when the embedded widget is used to create the target entity while editing the host. It wouldn't be set if the entity in the field exists in the database. target_id would be used instead.

I'm having difficulty verifying this because I haven't been able to find an explicit definition for it. I can include another version of the patch attempting to overload ->entity to see whether it will work or not, but I believe #8 would have passed if that was the case.

The story is a bit different when the field data makes its way to the formatter. ->entity needs to get set in prepareView() because entityReferenceFormatterBase->getEntitiesToView() depends on it being there. As for the elseif, I had sorta copied that from entityReferenceFormatterBase->prepareView() but I can see now that it isn't necessary as the new function covers both cases.

Status: Needs review » Needs work

The last submitted patch, 13: err-loading-2673076-13-reuse_entity.patch, failed testing.

jmuzz’s picture

Status: Needs work » Needs review

Not sure if this will stick or if testbot will change it back again but I'm going to try.

Berdir’s picture

Status: Needs review » Needs work

->target_id and ->entity are expected to always be in sync. A new entity is a special case which then doesn't have an id, otherwise, any kind of set and get access should work consistently for taget_id (in case of ERR, in combination with revision_id) and entity.

Maybe something goes wrong because you explicitly assign NULL to ->entity. Since there is a log of magic going on, this might have unexpected side effects. Try loading to a local $entity variable and only assign it when you successfully loaded the entity, and in the correct revision.

jmuzz’s picture

Status: Needs work » Needs review
FileSize
2.61 KB

->target_id and ->entity are expected to always be in sync.

Where do you see that? If that's true then why isn't it in sync when it gets to the formatter? EntityReferenceFormatterBase()->prepareView() does assign to ->entity.

Maybe something goes wrong because you explicitly assign NULL to ->entity.

If it was getting assigned NULL something certainly would go wrong in EntityReferenceFormatterBase::checkAccess() as the value would have been assigned to ->entity in prepareView and then when it attempts to

return $entity->access('view', NULL, TRUE);

the other patch in #13 would have failed too.

I'll give that a try though.

Berdir’s picture

+++ b/src/Plugin/Field/FieldFormatter/EntityReferenceRevisionsFormatterBase.php
@@ -13,14 +13,15 @@ abstract class EntityReferenceRevisionsFormatterBase extends EntityReferenceForm
+          $item->entity = $item->getReferencedEntity();

Yep, this seems to work :)

Why do you think they are out of sync?

this line is pretty useless now, since getReferencedEntity() already assigned that. I would leave it out but that important if you prefer it for clarity.

Lets make sure to verify that all paragraphs tests, especially the preview test still work with this.

Berdir’s picture

Status: Needs review » Needs work

Actually, I think we can implement this logic directly in \Drupal\entity_reference_revisions\Plugin\DataType\EntityReferenceRevisions::getTarget() because we have our own typed data reference class. That's what is called when you do ->entity and it isn't loaded yet. Then we don't need any special code at all and this behavior works all the time, without special methods.

jmuzz’s picture

I agree with #19.

Why do you think they are out of sync?

See #11 and #13. You stated that it was incorrect but I haven't seen the evidence that supports that. The question was meant to point out that your explanation about ->entity doesn't match the behaviour of the code.

jmuzz’s picture

I learned some things about the typed data API and I saw what you were saying about ->entity.

It isn't out of sync in the formatter. It takes the opportunity to load any entities that haven't been loaded yet as a batch since it's more efficient than letting them load by referring to their ->entity's separately.

Berdir’s picture

As mentioned to Miro, this looks fine, but I think our default revision handling isn't 100% correct yet, specifically for forward revisions.

We only ever enable default revision, and we only do so when saving a new revision (I think).

We need to always keep default revision flag in sync with the parent, even if there is not a new revision and just that is changing.

We possibly want to open a separate issue to write tests for that and ensure it works properly, and then we can get this in.

Andre-B’s picture

@Berdir @miro_dietiker any news on this issue? are there plans to leverage caching?

miro_dietiker’s picture

Status: Needs review » Needs work

I would want to know how many loadRevision() are skipped and what the impact is for a real world reference case...

Maybe we could mock things in a Unit test and make sure loadRevision is not called? That sounds complicated though.

Core seems to add support for load_multiple as well, which might have further impact.
#1730874: Add support for loading multiple revisions at once

At least, we should do some profiling in real or reference cases to know a bit its impact.
https://www.drupal.org/sandbox/mirodietiker/2855549
Berdir will look into this..

miro_dietiker’s picture

Pity, the performance tests are currently broken.. #2934590: Fix test fails

miro_dietiker’s picture

Aww they are working.. and fixed them. :-)

Note that these tests are focussing on edit performance, not only display performance.

Without patch:

Drupal\paragraphs_performance\Tests\UIPerformanceTest        1190 passes   6 fails                502 messages

Test run duration: 9 min 47 sec
0 user paragraph added. 0.231 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass
1 text paragraph added. 0.24 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass
2 image paragraph added. 0.305 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass
3 user paragraph added. 0.295 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass
4 image paragraph added. 0.319 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass
5 text paragraph added. 0.32 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass

With patch:

Drupal\paragraphs_performance\Tests\UIPerformanceTest        1190 passes   6 fails                502 messages

Test run duration: 9 min 50 sec
1 paragraph added. 0.276 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass
2 paragraph added. 0.255 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass
3 paragraph added. 0.254 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass
4 paragraph added. 0.351 sec	1	UIPerformanceTest.php	298	Drupal\paragraphs_performance\Tests\UIPerformanceTest->outputTimeStamps()	Pass
5 paragraph added. 0.285 sec

So it seems that with the patch, adding a paragraph even is consistently slower. :-)

Berdir’s picture

The paragraphs translation tests are failing with this apparently:

Drupal\paragraphs\Tests\Classic\ParagraphsTranslationTest 624 passes 2 fails 150 messages
Drupal\paragraphs\Tests\Experimental\ParagraphsExperimentalT 623 passes 2 fails 150 messages

Didn't yet check why exactly.

Berdir’s picture

Priority: Normal » Major
Issue tags: +Performance

I'm seeing a 20% faster page response on a client project with a huge amount of paragraphs, so setting to major. Still need to figure out why it breaks the paragraph tests and ensuring that the default revision is always set correctly or it will be a performance regression in those cases.

johnchque’s picture

Assigned: Unassigned » johnchque

Gonna check the Paragraphs test fails.

johnchque’s picture

I have tested this and I found something mostly related with saving the paragraphs.

The place where the test fails in paragraphs creates a node in the following way:
- Node in German with the API.
- One Paragraph in English with API. (The one that is failling)
- One Paragraph in German with translation in English wit API.
Then we enter to the node edit form just to save and check that the Paragraphs languages are being updated.
So all Paragraphs should be in German.

The problem is actually really strange since when debugging, the language is set correctly to the first Paragraph (language is updated to DE) when entering to the form but when it is saved, the paragraph that is loaded with the API in the test, keeps the language as EN.

Uploading the patch with debug messages for the Paragraphs module.

As we see in the image, the Paragraph with revision id 17 (The first paragraph) sets its language to DE, and the paragraph 18 switches to the translation. After, when loaded the paragraphs 17 language is EN.


(In the image we only see the language of the paragraph 17).

mtodor’s picture

I have investigated issues with paragraphs tests. Looks like the problem is not related to patch.

Here is relevant issue: #2961469: Failed tests for paragraphs translations.

Berdir’s picture

Assigned: johnchque » Unassigned
Status: Needs work » Reviewed & tested by the community

Thanks, lets get this in then in that case.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Fixed

Performance++ :-) Time to get this in then.

Updated related issue about default revision - we should make sure that default revisions are always correctly set.
#2903928: Default revision is not updated if host entity is not explicitly saved as new revision

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.