Lazy load multiple entities at a time using fibers

Subscribing, will try to comment soon. Overall in favor of entities-as-classes.

I don't think we would have

$entity = new Entity($type, $id); // $id likely optional so that mock entities can be created, potentially new ones.

Instead it would need to be a factory method (function or class method).

$entity = entity_load($type, $id);

or maybe:

$node = DrupalEntity::load($type, $id);

We'd have to think about creative use of __get() (as well as __isset())? Though I'm not sure if there would be a meaningful performance hit by having to copy all the properties from a loaded object into the stub object.

Instead it would need to be a factory method (function or class method).

Agreed.
I do think we want have per-entity-type classes like Node, Comment, .., which are useful for implementing all the entity-type specifics. See the WIP code at #1184944: Make entities classed objects, introduce CRUD support. This makes lazy-loading generic entity objects impossible, though I don't think this really is an issue.

* When ->load() (either directly called or via magic) asks the EntityLoad controller, it inspects the list of entity IDs that it has, vs. the ones that are already loaded. At this point, it is able to multiple load all of the nodes that weren't loaded already - so they're ready for later when requested. (we could also add a method or property to disable this behaviour, allow the default to be overridden or similar).

I'm not a big fan of this, because it makes it extremely hard to follow/control what happens. Some simple calls accessing a property could magically trigger massive load operations, even if the lazy-load is triggered by a simple read access of a property. That way it becomes harder for developers to know what's going on and to keep control.

I'd like to avoid magic getters or methods for the same reason - all the magic makes it hard for developers to see or track what happens. I've already gone that way with faces and the magic really just made things more complicated :( It doesn't help much if someone can simply call a method or access a property if no-one knows it's there in the first place; or if people read the code and don't actually understand what happens in the back.

- for lists, multiple load works more or less the same as now, except you could just foreach over $nids, instantiate the class and load it - no get $nids, load, then foreach pattern. So it should be a simpler for people who just want to load nodes and do stuff with them. The front-loading would be encapsulated in the class logic rather than a pattern that has to be copy/pasted around.

Actually, I'd prefer the pattern. A load-call really isn't hard, but it makes it obvious to me what happens. So in the end I'd argue the load call is simpler, because I don't need to be aware of any magic that happens in the back.

Still, imho having a single or multiple lazyload could be worthwhile as it could help to avoid some unnecessary entity-loads while leading to a more consistent and simpler API (no more Node vs NodeID function args or returned variables..).

I agree we probably do want to have a class-per-entity-type rather than one uber Entity class and that's it. However, that doesn't mean we can't still have a universal factory. The universal factory just needs to be told what type of entity it's dealing with along with the id(s). That's quite easy to do if we allow ourselves to mentally separate loading of entities from "being" an entity entirely; You don't get an entity from new Entity or new Node, ever, at all. You get it from a factory that does not begin with Entity:: or Node::. There's nothing wrong with that.

(As I said in a previous DrupalCon presentation, it is not the job of Pizza to make Pizza. It is the job of Pizza to BE Pizza.)

Also, in a previous project back in Drupal 5, which actually served as much of the inspiration for the Data API Design Sprint and the Fields/Entities system that resulted, I had the concept of a "load set". I was loading data objects from a 3rd party system into my own mini-ORM or mini-Entities system. By default, all objects were loaded with just an ID, say from a search result set. When you tried accessing data from one of them, it would lazy-load data. However, to avoid the Select N+1 problem it would lazy-load from *all* objects in that load set at once.

That is, if you got a list of 20 Artwork ids from a search query, you'd get back just IDs. The code would then create 20 Artwork objects, each one with just its ID, no other data, and a load set ID that linked all 20 of them together. No traffic here to load data from the remote system yet. Then when you first accessed data that would result in a network hit, it did the equivalent of node_load_multiple() for all 20 records and populated all of them at that point. Then all Artwork objects had full data, and the code could safely proceed in ignorance of that fact without triggering an avalanche of network traffic to the legacy system. (We later added caching to it as well.)

Perhaps we could explore a similar setup here. When you create entity objects, you create a set of linked skeleton objects. They then initialize and load en masse with entity_load_multiple() (or something similar to it) on first use. That gives you the double benefit of super-thin objects by default and multi-load to avoid tons of hits to the datastore.

I agree we probably do want to have a class-per-entity-type rather than one uber Entity class and that's it. However, that doesn't mean we can't still have a universal factory.

Indeed. I was thinking about lazy-loading the entity-type too, which has totally nothing to do with that discussion. So let's forget that, sry.

Perhaps we could explore a similar setup here. When you create entity objects, you create a set of linked skeleton objects. They then initialize and load en masse with entity_load_multiple() (or something similar to it) on first use. That gives you the double benefit of super-thin objects by default and multi-load to avoid tons of hits to the datastore.

Just to make that clear, I'd be happy having this kind of lazy-loading. I'm just hesitant about having the controller magically loading multiple entities while a single entity has been requested, as well as other __get() magic.

@__get():

We can do a get() method which allows easy access to properties and triggers all kind of lazy-loading the same way. So we have the same advantages, but it's obvious and easy to track for developers what accessing the property could trigger. Also we could allow using $entity->load() and directly accessing plain entity properties as usual afterwards without triggering any magic at all.

Personally I think the entity system would be greatly improved if we got rid of arbitrary object properties on entity objects. Have fields, have Entity Properties (by whatever API), and don't allow/support anything else. That's going to be necessary for CMI anyway for deployment, I think. If we need to track Entity Metadata as well (think flag status, nodequeue, possibly OG membership, etc.), that should have first-class support rather than being punted as "well, just throw random crap on the object and hope it doesn't break". That would also help deployment.

That would then give us very clear, explicit places where lazy-loading could be triggered, as well as various other advantages. (I'd like to see title turned into a method, for instance, on all entities.)

Also, $node->lazyLoadMe() and __get() are not mutually exclusive. To wit:

class Entity {
  public function lazyLoadMe() {
    $this->controller->lazyLoad($this); // Controller handles the lazy loading, but needs context.
  }

  public function __get($var) {
    $this->lazyLoadMe();
    return $this->$var;
  }
}

That's probably how we'd want to implement it anyway, so the only question is whether lazyLoadMe() is public or protected. I think in this case it's fine to be public.

catch: So you're suggesting that all Field data actually live in the controller object, and the entity object is just a routing shell? I am intrigued but not yet convinced. That seems like it adds a lot of unnecessary indirection in common usage, since then all requests go through __get() rather than just the first. __get() has been going out of vogue in the PHP world from what I've heard, because it makes code harder to follow/debug/auto-complete. (Don't underestimate the importance of the last one.)

Personally I think the entity system would be greatly improved if we got rid of arbitrary object properties on entity objects. Have fields, have Entity Properties (by whatever API), and don't allow/support anything else. That's going to be necessary for CMI anyway for deployment, I think. If we need to track Entity Metadata as well (think flag status, nodequeue, possibly OG membership, etc.), that should have first-class support rather than being punted as "well, just throw random crap on the object and hope it doesn't break". That would also help deployment.

Exactly, we need an "entity property API".

>catch: So you're suggesting that all Field data actually live in the controller object, and the entity object is just a routing shell?

As said, I don't like having $node->property involve lots of magic and I very much doubt it's good for performance. Once we lazy-loaded an entity, we should have as simple and fast access to the properties as possible.
Also we really need an object containing all the data, else stuff like entity form workflows are going to become even more complicated than they are now.

ad #12:
Registering entity objects in the controller makes sense to me, though we need to keep out entities that are currently edited, thus changed, but not yet saved.
However, most entity objects are going to be loaded via the controller anyway, except for the newly created (=saved) ones which we should register. Not sure whether we should just leave out un-serialized objects (-> form workflow). Maybe better this could be solved by creating a new revision before doing any changes (assuming that each edit generates a new revision anyway).

That's probably how we'd want to implement it anyway, so the only question is whether lazyLoadMe() is public or protected. I think in this case it's fine to be public.

Yes, at least it should be public. Give developers control. Restricting __get() magic to be triggered only for registered entity properties would certainly help to reduce unwanted side-effects. Still, I'm not convinced implementing __get() is a good idea.

Well, lazy loading that you have to explicitly initialize doesn't really buy you much. The developer then needs to know whether an object has been fully loaded or not, which defeats the purpose. So we're going to need some sort of automatic "OK, now I need to be loaded" trigger one way or another, and __get() is as good as any I suppose. I agree that we don't want to hit it for every property, though.

>Well, lazy loading that you have to explicitly initialize doesn't really buy you much.

Sure. Still, it doesn't have to be magic, but could be triggered by an explicit function call, e.g. $entity->get().

We probably need a getter like this anyway, as we want to assist users getting values of various languages, while using a sensible default.

E.g. I think a getter that consistently works like

$terms = $node->get('field_tags', 'en');
echo $terms[0]->label();

for all properties including fields what be helpful. Whereas in that example both function calls could trigger lazy-loading.

Using __get() to accomplish lazy field loading doesn't really have to mean a negative performance impact. In fact I think it will mean a positive one: __get() will be called only once, if we store the loaded field data inside the entity object during that __get() call.

There are lots of situations where we only need data of a couple of fields so we have a performance win for every field not loaded. An issue I see however is how to combine this with entity caching.

A required getter function like fago suggests in #16 is definitely going to be a performance hit; A function call is consuming at least double execution time as a property call. Fields are most of the time accessed multiple times during one request.

A cool benefit of lazy loading would be that you can immediately use entity objects returned by EntityFieldQuery.

Good idea. I was just thinking about the same thing the other way around. Allow fields to be preloaded while all others are lazyloaded, but suppressing fields might be better.

I think I agree with casey about __get() - it should be faster than having a method call every time, and this is one place where we could use some indirection - it will allow us to stop people accessing $node->field_name[$langcode][0]['foo'] finally as well as other goodies.

So how would this work then, considering translatable properties?

Yep, just going with the default language via __get() should do it + offer other languages via get()".

I've thought of calling get() only in case you are not sure whether the entity has been already loaded or not, but I see the beauty of not having to deal with that. As long as __get() is restricted to the list of defined properties it should work out.

Hum, subscribe.

Consensus at the code sprint was that we should postpone any consideration of this, especially gene the level of new WTFs that might be introduced and the unclear effect on performance.

The current issue summary makes a lot of guesses about how the classed entities will work, but that code is in now. So it needs an overhaul.

We also discussed yesterday with fago that this is related to the Entity Property API as well, where we might want to have an EntityProperty class that supports lazy loading it's property iems. Another use case might be taxonomy_get_tree() and similar functions that might return a possibly large number of entities?

Sorting out tags - this is not a configuration system issue

new RFC happening https://wiki.php.net/rfc/true_async

A couple comments on the MR that look like bugs, not sure if related to the Umami test failure.

It's easy to reproduce with the combined MR, you just have to clear caches and load the umami front page to see broken rendering.

Tested this out locally really quick and I can see that drupalSettings.bigPipePlacheolderIds has 3 unreplaced entries.

{
    "callback=Drupal%5Cblock%5CBlockViewBuilder%3A%3AlazyBuilder&args%5B0%5D=umami_views_block__recipe_collections_block&args%5B1%5D=full&args%5B2%5D&token=LSXAUcXQcqsNxZk01EJ-DceQbM_P3ovAc_wniqiM2X0": true,
    "callback=Drupal%5Cblock%5CBlockViewBuilder%3A%3AlazyBuilder&args%5B0%5D=umami_views_block__promoted_items_block_1&args%5B1%5D=full&args%5B2%5D&token=XKowAdwoi2xYmp7GHvIqQteOfgS9deS_4OJckMkYGHQ": true,
    "callback=Drupal%5Cblock%5CBlockViewBuilder%3A%3AlazyBuilder&args%5B0%5D=umami_banner_home&args%5B1%5D=full&args%5B2%5D&token=j1t-pelbFkxnp6XgWmQ0AG2woBVFHKrU7calg2QLe8M": true
}

FWIW, since #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers is in, I applied the changes to core/lib/Drupal/Core/Entity/EntityStorageBase.php from MR 12444 to 11.x and still see unreplaced big pipe placeholders on the Umami home page, even with the exclusion of file entities. Of note is that one of the unreplaced placeholders is the recipe collection view block, which does not contain any images.

It could be that I didn't cherry-pick the changes correctly, but I don't think that's the case.

Do file and media entities still have to be excluded after the Twig fix?

And should we attempt to remove a similar optimization from \Drupal\menu_link_content\Plugin\Menu\MenuLinkContent::$entityIdsToLoad here or in another issue?

Some quick notes from looking at this with xdebug a bit.

After a bit of trial and error, I ended up with a breakpoint after the suspend line with this condition $this->entityIdsToLoad && $ids != $this->entityIdsToLoad. So essentially I want to stop if there are entities to load and there are more than requested in the current fiber. This happens just twice on umami frontpage, once with a few nodes and once with a few files.

What's harder to understand and see is when it doesn't work out. I'm not getting anything on /en/articles for example, but that seems like the perfect example for this. 8 articles, each with a media and a file. is delayed rendering interfering with fibers, so that it happens outside or something like that? I can see that media render happens but there is no active fiber then. Ah, I forgot #3518668: Use Fibers for rendering views rows didn't happen yet. I'll try again with that, but that needs a rebase. not sure which makes sense to do first. but I guess both this and the path alias issue will see a major improvement with that, currently it only works in blocks or so?

because we merge entityIdsToLoad again, they're always duplicated, should we run a array_unique() already before we hit the memory cache instead of just in the else? I'm actually not 100% sure I understand the logic with that. it's also slightly weird to me that we load all the entities into $entities and then rely on the array replace at the end to only return the actually requested ones.

it's also slightly weird to me that we load all the entities into $entities and then rely on the array replace at the end to only return the actually requested ones.

Only vaguely remembering, but I think I had a comment on the other MR that array_replace() will add keys/values from the replacement arrays, so it might return more than the requested values? But that is just hypothetical, did not explore if this could actually happen.

See https://www.php.net/manual/en/function.array-replace.php#129944

re the array_replace() stuff. I don't think #75 is correct and I think it the current test only works because we only do a single load in each fiber. We should imho extend the test with an loadMultiple() call and make sure we only return requested entities.

If I set a breakpoint on umami with the breakpoint condition in #72 then I see this after I run through to the array_replace():

// The originally requested ids.
$flipped_ids = [3 => 0];
// The enriched $ids based on $this->entityIdsToLoad
$ids = [3, 18, 19]
// And $entities contains 3 items for 3, 18, 19 after the replace.
$entities = [3 => ..., 18 => ..., 19 => ...]

So, loadMultiple() definitely returns 3 entities when only one was requested. This only happens for the fiber that is resumed first, because for the others, $this->entityIdsToLoad has already been reset, so the merge there ends up with just the requested ids. That means to reproduce this, we'd need to have the loadMultiple() call first in the explicit test.

This doesn't fail on umami, because EntityStorageBase::load() explicitly returns the entity for the requested id and \Drupal\Core\Entity\EntityRepository::getCanonical() returns the first items, which is also guaranteed to be the requested one for a single load.

I think this looks good now, so +1 for RTBC. Thought I think @berdir mentioned he might want to test with #3518668: Use Fibers for rendering views rows, so not sure if that's still outstanding.

Added some comments.

I tested with the views issue, didn't see a difference yet, @catch mentioned that in #75 as well and that makes sense. This is by far the easier change, it works, and we then can enable it to work on more cases and test the impact then.

Forgot to mention that the issue title probably needs updating for clarity.

Test coverage is present. Here is output of test-only:

PHPUnit 11.5.39 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.12
Configuration: /builds/issue/drupal-1237636/core/phpunit.xml.dist
.FF....                                                             7 / 7 (100%)
Time: 00:29.009, Memory: 8.00 MB
There were 2 failures:
1) Drupal\KernelTests\Core\Entity\EntityApiTest::testLazyPreLoading
Failed asserting that false is true.
/builds/issue/drupal-1237636/core/tests/Drupal/KernelTests/Core/Entity/EntityApiTest.php:169
2) Drupal\KernelTests\Core\Entity\EntityApiTest::testLazyPreLoadingMultiple
Failed asserting that false is true.
/builds/issue/drupal-1237636/core/tests/Drupal/KernelTests/Core/Entity/EntityApiTest.php:215
FAILURES!
Tests: 7, Assertions: 244, Failures: 2.
HTML output directory sites/simpletest/browser_output is not a writable directory.
PHPUnit 11.5.39 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.12
Configuration: /builds/issue/drupal-1237636/core/phpunit.xml.dist
........                                                            8 / 8 (100%)
Time: 00:00.023, Memory: 8.00 MB
OK (8 tests, 24 assertions)
Exiting with EXIT_CODE=1

This looks good.

I added 2 review comments. +1 to @berdir's 2 suggestions. Hoping his suggestions have not been superseded by other ideas?

Writing out debug statements into files helps me understand what's going on, so I did that again here with this diff:

diff --git a/core/lib/Drupal/Core/Entity/EntityStorageBase.php b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
index 05aeb6587bb..bc5c15c464c 100644
--- a/core/lib/Drupal/Core/Entity/EntityStorageBase.php
+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -297,7 +297,11 @@ public function loadMultiple(?array $ids = NULL) {
// of entities to load, so that another call can load everything at
// once.
$this->entityIdsToLoad = array_merge($this->entityIdsToLoad, $ids);
+        file_put_contents('local/load/' . $_SERVER['REQUEST_TIME'] . '.txt', 'SUSPEND: ' . $this->entityTypeId . ':' . implode(',', $ids) . "\n", FILE_APPEND);
$fiber->suspend();
+        if ($this->entityIdsToLoad && $this->entityIdsToLoad != $ids) {
+          file_put_contents('local/load/' . $_SERVER['REQUEST_TIME'] . '.txt', 'BULK LOAD: ' . $this->entityTypeId . ':' . implode(',', $this->entityIdsToLoad) . "\n", FILE_APPEND);
+        }
// Another call to this method may have reset the entityIdsToLoad
// property after loading entities. Combine it with the passed in IDs
// again in case this has happened.
@@ -317,6 +321,9 @@ public function loadMultiple(?array $ids = NULL) {
// entity static cache itself).
$this->entityIdsToLoad = [];
}
+      else {
+        file_put_contents('local/load/' . $_SERVER['REQUEST_TIME'] . '.txt', 'SKIP: ' . $this->entityTypeId . ':' . implode(',', $ids) . "\n", FILE_APPEND);
+      }
}

// Try to gather any remaining entities from a 'preload' method. This method

so, I write out a line into a file in 3 cases. when suspending, with the requested ids. on resume, if we successful in aggregating multiple requests together and skip, when loading something and there's no active fiber.

On umami frontpage with disabled render cache, this results in

SKIP: user_role:authenticated
SUSPEND: node:10,9,8,7
SUSPEND: media:21
SUSPEND: user:6
SUSPEND: user:1
SUSPEND: file:41
SUSPEND: media:9
SUSPEND: file:17
SUSPEND: media:8
SUSPEND: file:15
SUSPEND: media:7
SUSPEND: file:13
SKIP: block_content:3
SKIP: block_content:1
SKIP: block_content:2
SUSPEND: shortcut:1,2
SUSPEND: node:3
SUSPEND: node:18
SUSPEND: taxonomy_term:1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16
SUSPEND: node:19
BULK LOAD: node:3,18,19
SUSPEND: media:18
SUSPEND: media:17
SUSPEND: media:19
BULK LOAD: media:18,17,19
SUSPEND: file:35
SUSPEND: file:33
SUSPEND: file:37
BULK LOAD: file:35,33,37

What surprised me a bit is that basically everything is in a fiber, that's because of the fiber in \Drupal\Core\Render\Renderer::executeInRenderContext(). the only exception is the 3 block_content entities being loaded as part of block access checking.

First are the nodes from the main page view and then they are being rendered, and this isn't working yet in parallel, so we'll need the views fiber issue or maybe #3548293: Use #lazy_builder / placeholdering for entity rendering for this.

Then it can load 3 nodes in bulk, then 3 medias and then 3 files. two nodes are from link fields and their access check on content blocks, and the third is from one of the views. I was confused why the second views block didn't show up, but that's because those two nodes are already loaded from the main view, those articles show up twice. medias are then a mix of one from the article and from the blocks.

=> Overall, shows that this works nicely. It aggregates all the things from all the blocks until it went through all of them, and then repeats. Even across different entity types as visible by the media case. Neat.

@berdir The 2x review comments you made are easy to implement. Not sure if @catch is needed to review them or if you can implement them and move closer to RTBTC? Thanks!

Re: #89

I'm so pleased that a wild plan from 2011 is finally coming together

Great work!

I always understood the benefit of fibre (porridge, all-bran);
fibers === drupal porridge?

I still think switching to $ids earlier would be easier to understand, but that's a minor issue, this is definitely better now.

The unit test changes are consistent with the other lines in that provider. Very weird, but consistent. Really not fond of those unit tests, but that's a different topic.

Lets do this.

Yes, exactly. It's a small change but at least for me I think easier to grasp what's happening. The $entities ternary could also be a regular if () now as the else is just reassigning $ids to $ids, but that's up to personal preference.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Ok, I think we're good here (again). Pretty excited for what this, the path alias and some other issues will allow us to do.

Speaking of that, @catch, I'm wondering if we should open an issue to "fiberize" \Drupal\block\BlockRepository::getVisibleBlocksPerRegion() to handle the b lock_content load entities. Maybe only some where access checks are non-trivial, I don't know what the overhead of fibers is. Some sites have a large amount of blocks. On umami, that would allow us to group those 3 block_content entities.

Created #3549088: Use fibers in BlockRepository::getVisibleBlocksPerRegion() as part of the meta, also grouping this under that meta.

Amazing to see this happen.

Committed a0378ef and pushed to 11.x. Thanks!