Optimise entity reference autocomplete widget loading referencable entities

Reviewing @DS.

I think a better solution would be to use a drupal_static variable to cache the results of an initial call, ala the patch I'm attaching. You'll also want to drupal_static_reset from places where the referenced list of entities can be saved, so that a form rebuild will display changes made, but I'll defer to more experienced minds before I get carried away implementing a full path. (I'll ping Kim and/or Lee on the Drupal Slack channel to confirm / correct).

#2:

Caching entities in a private / protected property could cause cache validity problems, some times even during the same request.
A better alternative is to make it through a dedicated cache service.

If you are aiming for static-like cache - consider the MemoryCache class in a custom service to inject and do the caching there.

You can take examples from the JSON:API module. Note that this is for Drupal 8.6+.

#8:

@NigelCunningham drupal_static should have already been deprecated in Drupal 8, so I think the patch in #8 is a no-go.

Based on the above I will be putting the issue back into needs work.

This is marked as a bug. Do we need tests on this?

Thanks @ndobromirov.

I wasn't aware that drupal_static was deprecated; good to know.

@dpi, I certainly see the validity of saying that Entity Reference Revisions should itself cache results at a lower level, but I don't think that in itself means it's invalid to cache the results at the widget level because the code in referencedEntities isn't trivial. Even if the cost of the loadMultiple was constant, without caching you've got n calls to referencedEntities, each involving 2 O(n) loops (unless the fields have no values at the time) and a ksort - still an O(n^2) algorithm.

On the other hand, putting static storage in the form doesn't seem to me to be a good pattern either - I think it makes most sense to put caching in referencedEntities itself, and use the MemoryCache class suggested above.

In cases for example where Entity Reference Revisions are used, referencedEntities actually loads revisions, which do not have a cache! (See #2620980: Add static and persistent caching to ContentEntityStorageBase::loadRevision()), so page load time and database queries increase

If this is going to be solved by #2620980: Add static and persistent caching to ContentEntityStorageBase::loadRevision() then I don't see any reason for the issue. There we have only 3 fails left. Please help us out there in order to make the revision loading more efficient.

Based on that I am closing the issue as "won't fix". Feel free to reopen in case I've missed something or the referenced issue is not sufficient in some way.

We should fundamentally avoid calling this method more than once if widgets are used in a multivalue context.

That is right. I am reopening it for that. So let's change

'#default_value' => isset($referenced_entities[$delta]) ? $referenced_entities[$delta] : NULL,

to

'#default_value' => $items->offsetExists($delta) ? $items->get($delta)->entity : NULL,

Turn @hchonov's suggestion in comment 15 into a patch; thanks Hristo!

There is no contract how the referenced entities will be retrieved. I don't see any BC issues. One might even argue that the ::formElement method should be explicitly working with the item at the corresponding delta. There is no other example where all values of all items are loaded at once like this. I would even dare to say that this method wasn't introduced for form processing.

Also calling ::referencedEntities multiple times without static entity cache is very inefficient. And it will be called multiple times for cardinality >1.

> Also calling ::referencedEntities multiple times without static entity cache is very inefficient. And it will be called multiple times for cardinality >1.

It's a little bit more complicated than that. Calling referencedEntities() means the field type has a chance to do a more efficient bulk load, which is indeed more efficient *if* it is statically cached. An assumption that's not true for ERR of course, but as I wrote above, we might be able to make it so without changing core. So using ->entity would be faster than using referencedEntities() now, but I think we can make referencedEntities() perform better than calling ->entity N times.

As for BC, it would in theory be possible to have a field type that implements referencedEntities() but does not have an ->entity property. ER and ERR both do, but some other field type might not.

As for BC, it would in theory be possible to have a field type that implements referencedEntities() but does not have an ->entity property. ER and ERR both do, but some other field type might not.

In theory this is possible. But here we're talking about \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget, which supports only the core own entity_reference field type. I don't see any contract in our BC policy about the methods that should be used in the implementation when retrieving the referenced entities.

Also if I as a developer expect that every :: formElement works with the delta item, just because every other field widget does that except this one, then I might expect that any alteration I do on the field item entity will be reflected on the widget, but this might not be the case if ::referencedEntities is used.

Yes, one can argue that there might be people extending from \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget and using entity reference fields without an entity property, but this is not something we should care about. This widget is bound to a specific field type in core and if one is extending from it, then they should be extending from the core own entity_reference field type as well.

Consistency is very important and if we can ensure it without performance boosts then it is better to provide it.

> But here we're talking about \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget, which supports only the core own entity_reference field type

If that were true, then this issue wouldn't exist. This is about the problems with the ERR field type, which has a slow referencedEntities() implementation. For core, using referencedEntities() might even be faster for multiple items because we can pre-load them all at once and then rely on the static cache.

You can define your own field type and make other, existing (core) widgets support it. Yes, it's kinda unlikely for my use case to exist, but still, if you did that in the past and updated core then your field type would be broken.

I'm not saying that's an absolute no-go to do this, but it's something that we need to consider and I still think that an improved referencedEntities() implementation in ERR would solve this too, improve performance in other cases that use that method (even if just called once, we could at least use loadMultiple() on the revisions, possibly do the same try-the-default-revision-first optimization as ->entity uses and avoid loading them twice).

Mistake

#3098924: referencedEntities() causes data loss is what I mean, that should reduce the overhead to a minimum as we'll just access the existing property.

I've created interdiff for last 2 patches. May it will help for next fixing. (GCI task).

I'm not clear why the widget and form need to do an entity load at all? I understand the autocomplete itself is doing an entity load to pull up ID and title based on the searched phrase, but when rendering the list, or saving the form, loading the entity doesn't seem to add any value and adds a tremendous amount of overhead, when dealing with a larger set of referenced entities.

This was a BugSmash bug bingo. It is an optimization and not a bug. I am changing to a task.

The status never got changed to Needs Review after the patch in #16.

(Though it needs to be added as a Merge Request, the changes should still be reviewed and tested.)

Can we turn to an MR and see what the pipeline says :)

Also could use a summary update as the proposed section is empty, what's being done to optimize

Have not reviewed but summary still needs to be updated.