Add an entity iterator to load entities in chunks

Love the idea! When we have a paging generator for the query we can achieve quite some stuff.

+++ b/core/lib/Drupal/Core/Entity/ChunkedIterator.php
@@ -0,0 +1,80 @@
+        yield $id => $entity;

generator are everywhere!

IMHO this is at least a task

Great idea!

I found a related project in D7 contrib https://www.drupal.org/project/entity_process_callback

Should be useful for CMI config entities as well as config ones. I wrote an iterator for 6.x Views at #853864: views_get_default_view() - race conditions and memory usage back in 2011 which still hasn't been committed :P

We've fixed the bulk of the issues in Views that patch was trying to solve, but for config updates we'll still need to load all and save, and there are potentially places like field info it could help as well.

I think this get more and more useful as we start to move away from batch API. For CLI you want to do everything in one process, and ideally wrap large operations like this in a transaction. So if we can write code that works in long running processes correctly like this, then use batch only to handle timeouts and user feedback, we'll be in a much better state overall.

1. +++ b/core/lib/Drupal/Core/Entity/ChunkedIterator.php
   @@ -0,0 +1,80 @@
   +class ChunkedIterator implements \IteratorAggregate, \Countable {
   

   I wonder if there is a common term for this in the world of software outside of Drupal?

2. +++ b/core/lib/Drupal/Core/Entity/ChunkedIterator.php
   @@ -0,0 +1,80 @@
   +   * @param int $cache_limit
   ...
   +    $this->cacheLimit = (int) $cache_limit;
   

   Wouldn't something like chunkSize make more sense?

I did a quick google for 'chunked iterator' and there at least people using that term on the internet, especially the last one is exactly the same issue we're dealing with here.

http://stackoverflow.com/questions/7926908/chunking-an-iterable

https://vinaybalamuru.wordpress.com/2012/05/14/basic-chunked-list-iterator/

I didn't send much time looking for other terms, worth doing but it wouldn't only be us at least if we stick with chunked iterator.

What I don't understand is why we need an iterator at all. Can't we just make a function which has the yield statement, aka. acts directly as generator and be done with it?
This generator would then implement \Traversable which is mostly all we need? Countable could then be something on top of that.

Well sure, let's get this in, its progress!

I think you should be able to achieve the same by using a class with a method in which you yield all the things.

As an API addition, this should now go into 8.1.x as per https://www.drupal.org/core/d8-allowed-changes#minor. Thanks @damiankloip and @dawehner!

+++ b/core/lib/Drupal/Core/Entity/ChunkedIterator.php
@@ -0,0 +1,83 @@
+    }
...
+    // Reset any previously loaded entities then load the current set of IDs.
+    $this->entityStorage->resetCache();
+    return $this->entityStorage->loadMultiple($ids);

Would it not just be better to have a way of loading entities without static caching at all? I think it is weird to blow the entire static cache even for entities not loaded through the ChunkedIterator.

Here is an idea, let's clone the entity storage object in the constructor. With that we no longer clear the cache on that particular object, so it doesn't affect the actual mostly used entity storage object.

\Drupal\Core\Entity\ContentEntityStorageBase::resetCache() clears both the static cache and the persistent cache. I can understand the iterator wanting to clear the static cache between chunks, but is clearing the persistent cache as well a good idea? I would think not, for the same reasons as #23.

Huh? When the hell did it start doing that? That method was always meant for clearing static caches only.

Content Entity storage seems to have had this feature ever since #597236: Add entity caching to core. \Drupal\Core\Entity\ContentEntityDatabaseStorage got it in the aforementioned issue. It was retained when \Drupal\Core\Entity\ContentEntityDatabaseStorage was renamed to \Drupal\Core\Entity\Sql\SqlContentEntityStorage in #2330091: Rename ContentEntityDatabaseStorage to SqlContentEntityStorage. It was then moved to \Drupal\Core\Entity\ContentEntityStorageBase in #2478459: FieldItemInterface methods are only invoked for SQL storage and are inconsistent with hooks.

We should definitely have a way to only reset the static entity cache. I had no clue this resulted in a persistent cache reset.

#2558857: Migrations invalidate entity caches when trying to reclaim memory, should flush has the same problem.

Also see #1596472: Replace hard coded static cache of entities with cache backends, #375494: Restrict the number of nodes held in the node_load() static cache, #1199866: Add an in-memory LRU cache.

So only the storage and tests themselves should have to worry about the persistent entity cache - just saving and updating field definitions etc. shouldn't need to reset caches at all. Feels like something we can straightforwardly change in a minor release at least.

So I think it's worth just deleting that code, moving it into ->save() somewhere, and seeing what breaks - probably separate issue to tackle that bit?

So, no movement on this issue in quite a while, but I'd very much like to see us approach this topic again as yields and generators are totally something we should be actively taking advantage of.

Phenaproxima and I were discussing this earlier today and in order to keep BC, he suggested a "streamMultiple()" method that can be used in leu of the current methods when yields are more favorable than returns. I'd also suggest a "streamMultipleByProperties()" method and just use raw entityFieldQuery with both of these and side-step all typical entity loading operations. I'm a little uncertain of the implications of the caching here, but since this use case is most beneficial in the scenario where we'd exhaust memory on the system by loading all the entities into memory in the first place, it seems beneficial to me to side step it completely if possible. Please feel free to disabuse me of that notion of it's senseless, I don't claim to understand all the nuance here.

Eclipse

+1 what @EclipseGc described. The idea of a specialized, chunked entity iterator, IMHO, is utterly obviated by the existence of generators. We should take advantage of this wonderful, criminally underused feature of PHP.

I propose we add an interface that can be optionally implemented by entity storages, to maintain BC:

interface StreamingEntityLoadInterface {

  public function streamMultiple($ids = NULL);

  public function streamMultipleByProperties(array $properties);

}

Both of these methods would return generators.

+1 what @EclipseGc described. The idea of a specialized, chunked entity iterator, IMHO, is utterly obviated by the existence of generators. We should take advantage of this wonderfully, criminally underused feature of PHP.

Did you had a look at the actual patch? This is really just using a generator, while it allows you to specify the chunk size. Putting more things onto the same object (entity the storage) makes it harder to maintain BC etc.

The tricky bit is really all about the reset of the caches, so you don't end up with infinite amount of used memory :)

So, at the risk of getting pedantic...

Yes I read the patch. Maybe we shouldn't lead with "did you read the patch"? It's kind of belittling.

In my comment, I never suggested that the patch wasn't using generators, just that we should see movement on this basic idea again because of the value of generators, thus the reason I even bothered to comment.

To the meat of the patch itself, my comment was really just trying to say "this is great but I feel like a simpler implementation might get quicker traction." and "Is it possible to side-step the caching completely?" (which is the conversation I'd prefer to be having) I'm not trying to start fights here, just trying to figure out what the mvp for a generator based solution to entity loading might be.

As catch explained to me in irc, loadMultiple exists for a reason, and that getting individual entities is more costly from a time perspective on long runs. I totally appreciate that. However, as Damian's own comment points out, whether we're loading one entity at a time, or 50... is sort of an implementation detail in this instance, so the least effort to get a generator of any sort into this pipe-line is probably a worthwhile effort.

If we don't want to add additional methods to the Entity Storage, that's fine. Again, implementation detail.

Eclipse

I have a hard time extracting what you try to tell us :)

lol ok tl:dr;

• I never suggested the patch wasn't using generators, obviously it is.
• I want to use generators asap.
• Is this patch truly the MVP to do so? Or can it be simpler and get in faster?

Eclipse

Well, keep in mind that in Drupal core MVP is tricky, as well, you have to support it for years. Putting some thoughts into the implementation, which @damiankloip did, is not a bad idea.

Reminder that I have a very old issue that would fix the caching problem here. Adding to related issues.

Yep that issue, I tried to add it as a related issue but didn't stick, trying again.

Duplicate of this issue #2895215: Provide an implementation of loading all entities with an iterator instead of an array of entities.

I guess since the Entity Memory Cache is now a consolidated service, we could move on here. Seems the only thing we need is to add this service and use it for clearing the cache, instead of (mis-)using $storage->resetCache().

Yes, let's see about rolling a new version of the patch.

\Drupal\Core\Cache\MemoryCache\MemoryCache is shared across all storages and in order to remove a specific entity from it one must know the CID under which the entity is cached, which currently is internal to the storage.

I think that a better solution will be to prevent adding the entities to the static cache in the first place. The following code should do the work:

// Turn off the static cache temporarily.
$entity_type = $storage->getEntityType();
$static_cache_enabled = $entity_type->isStaticallyCacheable();
$entity_type->set('static_cache', FALSE);

// Load the entity bypassing the static cache.
$entity = $storage->load($id);

// Re-enable the static cache.
$entity_type->set('static_cache', $static_cache_enabled);

#58 +1

re-roll using suggestion from #58 plus interdiff

NR please

Looking at Views module, which is where the pain point is for me.

Basically we need this to run in chunks in some way, basically as we iterate over $view->result.

\Drupal\views\Plugin\views\query\Sql::loadEntities()

I think what's a bit tricky there is that you don't really want to modify the result since you don't want the entities to stay in memory, but I don't feel as though it's clear as to wether that result will be iterated more than once.

This also seems like a bit of an API change since the result will be an iterator, not simple array?

I'm not sure how this should work for views, as views loads all those entities and puts them on the row objects, including relationships and so on an will need them at arbitrary locations as it is rendering them. I think this is more about bulk processing and background jobs, where time doesn't matter as much as memory usage.

re-roll using suggestion from #58 plus interdiff

Thanks for implementing it. The interdiff looks good.

Additionally I think we should ensure that $entity_type->set('static_cache', FALSE) always disables the static cache. This helps in case the entity type implementation is changed. Maybe an assertion would be enough?

$entity_type->set('static_cache', FALSE);
assert(!$entity_type->isStaticallyCacheable());

#63 maybe an API addition - a setting on the view to designate the rendering behavior: full result VS iterator based one. Then you are free to change as much as you like, as long as the current behavior is the default one.

Looking at this issue timeline, wouldn't it be a good choice to implement the bare-bone feature first, then think about the PITA it'll be to make it work with views?

BTW, chunking without any kind of steaming with Views would be as much irresponsible as not doing so with too many entities, particularly in a HTML context - see how Chrome behaves in a really, really, really large page with rows, forms and interactions. The kind of stuff many folks use together with Views.

I'll be happy to give a hand in documenting this

Resolved some coding standard issues & added an interdiff as well.

Re #67

Looking at this issue timeline, wouldn't it be a good choice to implement the bare-bone feature first, then think about the PITA it'll be to make it work with views?

BTW, chunking without any kind of steaming with Views would be as much irresponsible as not doing so with too many entities, particularly in a HTML context - see how Chrome behaves in a really, really, really large page with rows, forms and interactions. The kind of stuff many folks use together with Views.

I'll be happy to give a hand in documenting this

The issue here is not about views and the performance problems there, but about something completely different and it is described in the issue summary. It would be great if we could concentrate on the described feature here and for the views problems we would need a dedicated issue. I also don't see how views could benefit by an iterator as proposed here, but I would also rather not discuss that here.

I quote @Berdir from #64:

I think this is more about bulk processing and background jobs, where time doesn't matter as much as memory usage.

I think that we need to add the ability to load entities by revision ID as well.

+++ b/core/lib/Drupal/Core/Entity/ChunkedIterator.php
@@ -0,0 +1,82 @@
+   * This will also be the amount of cached entities stored before clearing the
+   * static cache.

This comment is obsolete, because we don't put the entities in the static cache with the newest approach.

+++ b/core/tests/Drupal/Tests/Core/Entity/ChunkedIteratorTest.php
@@ -0,0 +1,224 @@
+    $iterator = new ChunkedIterator($this->entityStorage->reveal(), [1, 2, 3]);

Instead of having to manually instantiate the iterator we could return it by the entity storage - $entity_storage->getChunkedIterator(). This will make it possible to define custom iterators for the custom storages if there is the need for that.

The issue here is not about views and the performance problems there, but about something completely different and it is described in the issue summary. It would be great if we could concentrate on the described feature here and for the views problems we would need a dedicated issue. I also don't see how views could benefit by an iterator as proposed here, but I would also rather not discuss that here.

I do agree with that. It wouldn't be particularly hard to create custom versions of iterators in general. Having one for views itself might be useful, but it's tricky given code will always assume that there are real arrays.

I think that we need to add the ability to load entities by revision ID as well.

This seems something which could be moved to a followup.

Reading this comment I was wondering something though. Looking at the API of the constructor:

+ public function __construct(EntityStorageInterface $entity_storage, array $ids, $chunk_size = 50) {
it makes it tricky to extend. Adding revision IDs afterwards could be tricky, changing the IDs while you iterate might be tricky.

I'm wondering whether we could change the API a bit:

• Remove $ids from the constructor
• Add a method to set the entity IDs used in the iterator->setEntityIds()

What are your thoughts?

+++ b/core/lib/Drupal/Core/Entity/ChunkedIterator.php
@@ -0,0 +1,82 @@
+    foreach (array_chunk($this->entityIds, $this->chunkSize) as $ids_chunk) {

array_chunk is not memory efficient (at all). Especially when chunks get too smal and original list size is huge. Worst case: chunks of size 1-2 and some good 200k+ IDs. At that point you are easily spending 150MB+ on generating the chunks only. This is memory that is on top of keeping the whole list of original IDs in memory.

The snippet from here is tested in some production sites and it scales very well, memory wise.

If Drupal has something like ArrayUtils, it could go there.

+++ b/core/lib/Drupal/Core/Entity/ChunkedIterator.php
@@ -0,0 +1,82 @@
+  /**
+   * Loads a set of entities.
+   *
+   * This depends on the cacheLimit property.
+   */
+  protected function loadEntities(array $ids) {
+    // Turn off the static cache temporarily.
+    $entity_type = $this->entityStorage->getEntityType();
+    $static_cache_enabled = $entity_type->isStaticallyCacheable();
+    $entity_type->set('static_cache', FALSE);
+    $loaded = $this->entityStorage->loadMultiple($ids);
+    $entity_type->set('static_cache', $static_cache_enabled);
+    return $loaded;
+  }

The problem for me with this one is that you will not load the nodes, but some third party code will expect to have that node loaded as well. Think of path auto, tokens, url generation. They do their own entity loading, so if the iterator is used to iterate and modify (very common) the items we pass through it, the static cache will very commonly explode again. I think that the static cache should be disabled for the entity not only during single load, but through-out the life-time of the iterator.

I've had a case to debug such "memory leaks" on a project and it was not fun searching for them in a complicated D8 site.

I'm wondering whether we could change the API a bit:
- Remove $ids from the constructor
- Add a method to set the entity IDs used in the iterator->setEntityIds()
What are your thoughts?

If the method is returning $this should be ok, as it will allow chaining and will allow to write the iterator as an expression.

Re #73:

Add a method to set the entity IDs used in the iterator->setEntityIds()

And when we add setEntityRevisionIds() later, we have to forbid calling both methods, right? Also depending on which method has been used the keys returned by the iterator will be either entity IDs or entity revision IDs.

I guess we have only two options here - either using a setter method or having two different chunk iterators and retrieving them through different entity storage methods - getChunkedEntityIterator() and getChunkedRevisionIterator(). In the later case we could still use a dedicated constructor for both iterators.

Re #76:

The problem for me with this one is that you will not load the nodes, but some third party code will expect to have that node loaded as well. Think of path auto, tokens, url generation. They do their own entity loading, so if the iterator is used to iterate and modify (very common) the items we pass through it, the static cache will very commonly explode again. I think that the static cache should be disabled for the entity not only during single load, but through-out the life-time of the iterator.

After reading that suggestion for the first time I was totally for it. However after thinking a little bit more about it I am not that convinced anymore. The reason is that people can rely on the static cache and if it suddenly isn't there where they expect it to be then problems might arise. The idea of the chunked iterator is to prevent filling caches for the entities that are being retrieved through it. Yes, it might happen that in the middle between chunks the same entity is loaded again from the storage, because of some code that you are executing, and as a consequence put into the static cache, but this is not really a problem of the iterator itself.

It might even happen that you interrupt the iteration and then there is no way to re-enable the static cache of the storage. We might use the destructor for that, but it will be invoked either if you destroy the iterator instance manually or the function returns.

The chunked iterator is not for regular code, but for updates.

There is a more elegant solution to the problem without a dedicated class for the chunked iterator - by leveraging generator delegation:

public function getChunkedEntityIterator(array $ids, int $size = 50) {
  foreach (array_chunk($ids, $size) as $chunk) {
    $static_cache = $this->entityType->isStaticallyCacheable();
    $this->entityType->set('static_cache', FALSE);
    $entities = $this->loadMultiple($chunk);
    $this->entityType->set('static_cache', $static_cache);
    yield from $entities;
  } 
}

Another way of disabling the static cache would be by cloning the storage and exchanging the memory cache backend with a NullBackend:

public function getChunkedEntityIterator(array $ids, int $size = 50) {
  $storage = clone $this;
  $storage->memoryCache = new NullBackend('cache');
  foreach (array_chunk($ids, $size) as $chunk) {
    yield from $storage->loadMultiple($chunk);
  }
}

I prefer the generator delegation over the current solution, but I am not sure which is the better way of temporarily disabling the static cache - through the entity type or by exchanging the cache backend. What do you think?

Edit:
We can skip cloning the storage by simply swapping the cache backend before and after loading the entities.

Note the "cloning the storage and exchanging the memory cache backend with a NullBackend" is my idea and hchonov said he is not comfortable taking credit for the idea of someone else, well, now here I am, so if you decide to go with that one, you could credit me. Thanks for your consideration, this is a great, great issue, our organization could use it quite a bit, one of our sites have a few million Message entities, for example.

+++ b/core/lib/Drupal/Core/Entity/ChunkedIterator.php
@@ -0,0 +1,82 @@
+      foreach ($this->loadEntities($ids_chunk) as $id => $entity) {

Why not yield from $this->loadEntities($ids_chunk) instead of a second foreach?

I think that's what #79 suggests isn't it?

Can anyone comment on how this issue is progressing please. It would be very useful, I think, in fixing a problem in google_calendar, where potentially many thousands of events are being processed frequently. The problem is that the current entity API doesn't appear to let the code load only specific entity properties, while loading them all could result in arrays exceeding 100MB in size.

#3110585: Improve cleanup logic

Iterating the list would fix that. Would it be sane to consider using code in this issue inline in the module until it is supported by Drupal 8 core?

There is a more elegant solution to the problem without a dedicated class for the chunked iterator - by leveraging generator delegation:

Much like having a dedicated class feels a bit too much, I do wonder whether adding it to the storage class is the right idea either:

• What does this mean for the existing interface, we can't just expand it
• It also seems like the iteration is a orthogonal concept to the pure storage

I wonder whether the entity repository would be a good place to put this too?

For now I'm updating the patch with a bunch of points:

• It is using the memory cache instead of resettting the static cache is much better, as agreed above
• In my experience using a similar iterator in projects we often run into issues with entity references being loaded, and as such filling up the memory over time as well. This change removes all entity static caches after each iteration, what do you think?

@dawhener thanks for that.

I don't know enough about the various caches. I'm going to give it a go, but are there any things to watch out for, or places I can trap to check that all is well?

My understanding is that the static cache is a cache of entities that have been loaded from "upstream" (eg DB) to avoid the need to reload them from presumed slower storage methods. Looking through the code, I cannot see any limit imposed on the number of entities cached, which is why when dealing with thousands of non-trivial entities memory consumption can go very high.

[Aside, wouldn't it be a good idea to include some sort of garbage collection so entities can be evicted from cache early, or is there a problem with knowing what's changed and whether if it has been if it should be written? Would that be solvable with a per-entity-instance flag which is set on load to be the "write intent": readonly or writable, only readonly can be evicted, and the flag can be set writable at any time.]

My understanding, too, is that the point of resetting the cache was to clear out previously loaded entities, so rather than loading more and more and... we load, expunge, load, expunge.

With the change to use the memory cache, we are putting the just loaded cached entities into explicit memory store, rather than, e.g. memcache via the static cache interface? That's what is happening in your getIterator, after yield.

@rivimey

This issue is basically just a DX improvement, you can already do this with a bit more custom code. Some quick inputs:

* Yes, you should definitely never load an unlimited amount of entities, chunk it up and then load it in batches. array_chunk() is a useful helper for that.
* You should also consider to use the batch and/or queue API if you don't have to process everything in a single request.
* Entity storage now uses a separate memory cache bin that you can reset: https://www.drupal.org/node/2973262 (\Drupal::service('entity.memory_cache')->deleteAll(). The resetCache() method on storage also clears the persistent cache, so you don't want to use that.

Would someone be able to provide a simple explanation of this patch?

What do I expect as a developer / site builder? Seems like core maintainers may be more familiar with a lot of this, but I'm not. :(

I think my main question is around if this is adds a new feature that is available, but unused by default or if it's overriding something. Part of this confusion is that Drupal 8/9 has so many ways to register things.

Possible explanation 1:
- This adds a class `ChunkedIterator`
- If applied by itself, nothing happens
- If a module / core wants to use this behavior it can get an iterator with `$iterator = new ChunkedIterator($entityStorage, $memoryCache, [1, 2, 3]);` and then ... do something?
- Memory usage is reduced

- Questions: Entity storage is from the `entity_type_manager`, how do I know what to provide as `memoryCache`? Can someone provide some pseudo code or small sample of how to use this? Maybe that could be included in the Class comment as well.

Possible explanation 2 (I don't think this is happening):
- This adds a class `ChunkedIterator`
- Something is swapped out in core
- Existing systems like `$entity_storage->loadMultiple()` will use this...
- Memory usage is reduced?

....

If it's helpful, our current challenge is that our JSON API requests are taking a lot of memory. We are trying to understand if part of the issues is loading tons of entities into memory unnecessarily. We haven't been able to review this yet. If it is an issue, we want to look at other options that require less memory.

Thank you!

It's definitely explanation 1. I wish we could do something more like explanation 2, but this would require a different sort of interaction pattern with the returns than we have today. Maybe a contrib module could introduce a "stream_storage" handler that yields values instead of building big arrays first. It would be a huge change to introduce into core on the existing storage handlers.

Also, I'd say this is mostly for batch/CLI/background processes, if you are loading that many entities in a regular request then you're already in trouble. You trade in lower memory usage for slower processing (it's doing multiple loadMultiple() calls over time).

And in the end, it's just a DX improvement, you can already fairly easily implement it yourself, array_chunk() your ID's and call the entity memory reset at the end of a loop (\Drupal::service('entity.memory_cache')->deleteAll();)

@damontgomery for memory issues on a regular request, look at issues like #1199866: Add an in-memory LRU cache and #3190992: Add a WeakReference memory cache implementation. However like Berdir says the best thing to do is to try to find a way to load less entities during http requests in the first place.

@Berdir, @catch to be clear, the use case I have in google_calendar module is this:

Google's Calendar API is a little strange. It forces the client to reload all events, possibly multiple times per day, rather than explicitly creating and deleting the calendar events in question. Doing a full reload of events is the only way to change the fixed time period you are loading events for, and if you don't specify a time period, you are potentially reading an actually infinite number of events. The Drupal module's purpose is to create fieldable local Drupal entities for events, and tries to do so such that any given event only ever causes an entity to be created once. It is necessary, therefore, to calculate the intersection of local vs remote events.

Calculating the intersection of events requires loading all local event entities and 'checking them off' with each remote event as it is streamed in chunks from Google. So we read in potentially 10,000 non-trivial entities and mostly throw them away again immediately. Google entities have a UUID-like string which identifies them uniquely so it is this we are comparing... most other fields are irrelevant. This process is typically repeated once a day as a cron task. What is worse, for complex reasons I cannot see any way to avoid needing to do this whole process twice per update.

I would very much welcome any thoughts on how to streamline the process to reduce both (a) the total memory consumption, and (b) the time taken in loadMultiple calls.

The best option for me would be a way to either:
(a) specify which specific entity fields are required in loadMultiple (such that no other fields are even read from the DB), and/or
(b) to extend the SQL filtering and join options available during loadMultiple, such that more of this can be done in the database (which is designed for such, operations and also has much of the data "to hand"). At present the entity API interface is not capable of doing what's needed, and while custom queries could be done that brings a real chance of breaking things.

This issue has nothing to do with partial loading of entities entities or changing how entities can be loaded. The only thing it could help with is that you could ask it to load 1000 entities and then loop over it and it would load them in chunks of 50 or 100 or whatever. But that's as I said just a DX nicety that you can essentially implement yourself with one extra loop and array_chunk()ing your list of ID's, combined with an entity memory cache reset.

I'm not aware of any plans to implement partial loading of entities or extend our query capabilities. Since you don't just want to know if an event exists but also if any relevant properties changed, the only alternative to loading them and comparing in PHP that I can see would be to do an entity query for every single one with a big OR condition where you compare each field, if it matches then it doesn't need to be updated.

The other idea I have would be to split the configured time frame into smaller ones and create a batch item for each twice a day or however often you want. Then Drupal could continuously work through that and people could use more efficient batch processing tools like a ongoing separate process. But I'm I don't know if that's actually desired and could have really weird side effects too (moving an event?).

@Berdir, thanks for the thoughts. I hadn't considered multiple time frames, but I think you're right -- there would be a lot of side effects to take account of, and it would significantly complicate the logic. I'm not really sure I understood the other suggestion (the big OR). Such a query would be truly massive if it could be created. I'm not joking about 10,000 entities.

I'll leave this be here, however, and thanks for your consideration.

I mean 10000x one query. You store a hash of all the relevant values that you are keeping in sync. You query for the ID and hash, if you have a match, it is up to date. If not, you need to load it to see if it exists and if yes, update, else create. Or can put a method on your storage handler that does an optimized query for that. You could say select id, hash where id in (id1, id2, id3) in certain chunks, you don't want to do it with 10k ids.

It is your entity type, so you can add a custom storage handler with a method and an optimized query for that, or a repository service like I'm doing in redirect.module.

@hchonov @Charlie ChX Negyesi

I can totally see the usecase for adding a method to the entity storage. Regarding the implementation of swapping out $storage->memoryCache = new NullBackend('cache');, it's a bit problematic, as you can easily load references to other entities.
One example I've seen in production environments are the owner of an entity. You don't want to keep these entity around for long either.
Does that make sense for you?

@EclipseGc

It's definitely explanation 1. I wish we could do something more like explanation 2, but this would require a different sort of interaction pattern with the returns than we have today. Maybe a contrib module could introduce a "stream_storage" handler that yields values instead of building big arrays first. It would be a huge change to introduce into core on the existing storage handlers.

Reading out this I was wondering whether it is possible to implement something in between. Maybe this class could easily support not just an array, but also any iterable. https://git.drupalcode.org/issue/drupal-2577417/-/commit/4c812b239b4adbe... implements this.

Could this be tied more closely to EntityQuery?

It's not that hard to exhaust memory with a very large array of IDs.

@joachim entity query only returns entity IDs, it's usually when the results are loaded that we get into issues.

There's also #1199866: Add an in-memory LRU cache which would limit memory usage from all entity loads.

> @joachim entity query only returns entity IDs, it's usually when the results are loaded that we get into issues.

Yes, my point is that when you load an array of LOTS of entity IDs, you can exhaust memory too. IIRC it was about 1-2 million.

What about passing the query object to the iterator, so it can handle getting the query result in chunks?

Reroll patch against Drupal version 9.3

patch on #110 creates new file outside of the core
Fixed Failed Commands #110
Attached reroll patch against 9.5.x

Fixed Failed Commands on #111
Attached interdiff

Ignore #112
Fixed Failed Commands on #111
Attached interdiff

This should address the test failure.

+++ b/core/lib/Drupal/Core/Entity/ChunkedIterator.php
@@ -0,0 +1,56 @@
+   * @param \Drupal\Core\Cache\MemoryCache\MemoryCacheInterface $memoryCache
+   *   The memory cache.
...
+  public function __construct(protected EntityStorageInterface $entityStorage, protected MemoryCacheInterface $memoryCache, array $ids, protected int $chunkSize = 50) {

Cant call array_values on any iterable.
Dont think iterables are necessarily countable..?

1. Comments on the MR were resolved, array $ids replaced iterables $ids, +1 to this change.

2. Read most of the comments here. The cache was discussed a lot. As a dev to use the iterator, I really do not want to care about cache, as long as it works.

I would use it like the following:

// For example, the entity type is `node`.
$iterator = new ChunkedIterator('node', $ids, $chunk_size);
foreach ($iterator as $loaded_entities) {
  // do something with $load_entities.
}

Instead of passing in $entity_storage, giving $entity_type is simpler. we can default $memoryCache to \Drupal::service('entity.memory_cache')

Suggested change:

public function __construct(protected string $entity_type, array $ids, protected int $chunkSize = 50, protected MemoryCacheInterface $memoryCache = NULL, protected EntityStorageInterface $entityStorage = NULL ) {
  if($memoryCache === NULL) {
     $this->memoryCache = \Drupal::service('entity.memory_cache');
  }
  if($entityStorage === NULL) {
     $this->entityStorage = \Drupal::entityTypeManager()->getStorage($entity_type);
  }
}

3. MW for Needs CR at least.

Good idea overall in this issue!

Btw, sometimes we want to not just save memory, but also split the operation into multiple requests.
That is, we want to pause somewhere and resume later.
I think this mostly applies for content entities, probably never for config entities.

An obvious way to do this would be paging. But this would break if entities are added or removed between requests.

Instead, for entities with auto-increment ids, a solution can be to load them in reverse order of id, and remember the last processed id (or next id to be processed) between requests. This guarantees that all entities will be processed that existed when the operation was started.

To do this, the generator method could accept an additional parameter to indicate a starting point (min/inf or max/sup id).
Or it could be passed through an additional method (setter or wither).
Or there could be a factory for the entity iterator.
Or it could be something we add in the entity query classes.

---

Currently the proposed class can not easily be injected as a dependency or registered as a service, because the ids iterator has to be provided at construction time.
Passing the ids iterator directly to the generator method would work, but then the class is no longer an IteratorAggregate, so no longer works in foreach() directly.
Another option is to start with an empty list of ids, and use a wither method (= immutable setter that returns a modified clone) to set the ids. So the initial object would be technically complete, but useless, without the ids being added.

Also, for a generator, we don't _need_ to implement `\IteratorAggregate`, we can also have a generator method with a more custom signature. Of course then you can't just foreach($object as ..) it, you have to do foreach ($object->generatorMethod()).

what's left here except CR?

I still think this needs to be tied closed to EntityQuery - arrays of IDs aren't scalable either.

Also, instantiating the class needs to be made easier -- having to pass in a cache object isn't good DX.

It seems like @donquixote and @joachim are bringing out some valid points:

- Naming
- Developer experience
- Genericness

Generally I agree that the experience is not great. It feels like having some form of factory for that seems a good idea.
A few places for that are:

- Entity storage class (adding even more to those classes seems hard)
- Entity repository (feels like a nice place) (createEntityIterable(iterable $ids): EntityIterable)
- Tie it to an entity query, like a method ->executeAndLoadIterable()

My general gut feeling is that entity repository is a good place. It balances flexibility with discoverability . We could then put in something on the entity query as a follow up on top of that.

Generally I agree that the experience is not great. It feels like having some form of factory for that seems a good idea.

We can use static methods to avoid polluting other interfaces.
We just pass in the required services or objects to the static methods.
I will try to come up with something.

Correcting myself:

Currently the proposed class can not easily be injected as a dependency or registered as a service, because the ids iterator has to be provided at construction time.

Actually this is not a case if the `iterable $ids` is an array or IteratorAggregate instead of an Iterator.
Maybe we should be more strict about the parameter.
We also then need something that can give us this list of ids, unless we want to use an array of ids always. This could be done in a follow-up.

So you are saying for now it would be better to:

- open up a follow up to accept an iterable
- accept only an array right now?

I’m excited to see what you come up with on the factory/initialisation side of things.

I wonder: Can we restrict this functionality to content entities with integer ids?
Usually for config entities it is fine to load them all at once.

This would mean we can provide more restrictive @param and @return docs.
E.g. @param list<int> $ids and @return \Iterator<int, ContentEntityInterface>

Content entities are not limited to int IDs, you can have strings as well there.

Content entities are not limited to int IDs, you can have strings as well there.

Alright, good to know!
Then we can as well support all entities including config, we would not gain anything from restricting it to just content entities.

------------------

To another point (@joachim in #106, #108).

Yes, my point is that when you load an array of LOTS of entity IDs, you can exhaust memory too. IIRC it was about 1-2 million.

I talked with @dawehner whether we only need to limit the entity objects loaded at once, or also the entity ids.

With millions of ids, the memory usage will still be survivable, though not negligible.
The ids are keyed by revision id, so this is not a list but a numeric associative array.
With memory_get_usage() compared before/after, I find that an array filled with $array[$i * 2] = $i for $i from 0 to 999.999 fills up 33.554.496 bytes in one system, and 41.943.120 bytes in another.
Besides the memory impact there is also a time cost, because Statement::fetchAllKeyed() uses a foreach() to fill the values.
In addition, we have to assume there is memory usage in the database engine. I don't know if the entire result set will be in sql memory at once, though.

I think we want to support the following cases:

• A long-running operation that really wants to process all entities in one go. It wants to avoid having all entity objects in memory, A long list of ids is usually ok, unless we accumulate copies of this list in logs or other places if things go wrong repeatedly.
  It could still be good to load the ids in chunks of 1.000 or 10.000 just to be respectful to other operations that need memory.
• A batch operation where in each step we process a limited number of entities. This could either be a fixed number, or the batch step could be timeboxed.
  I think most batch operations I have seen pre-compute the list of ids before starting the batch. Usually this means they will be all in memory at one point.
  Personally I think it is more elegant and scales better to use a query that can be resumed at a given offset, instead of relying on pre-computed ids. Here a chunking of ids can be useful, if the calling code does not already specify a range limit.
• An operation that only needs the first n entities, and does not care about the rest.
  Normally this will already put a range limit on the entity query.
  But what if the end is determined dynamically? E.g. list products until the accumulated price reaches a given limit. Here chunking of ids can be useful, especially if we run this more than once in a request, for whichever reason. But it seems quite rare.

So, it seems that, in most cases, it is ok or survivable to load all ids at once.
But the chunking or incremental loading of ids is probably easy enough to do it anyway, when there is not already a range limit. There is a benefit at least in some cases.

To do this properly, we should provide incremental loading of ids directly in the entity query.
We cannot add this to the existing query interface, but we can add an extended interface for this purpose.

> So, it seems that, in most cases, it is ok or survivable to load all ids at once.

That's not been my experience working on migrations -- see #3158436: Give batch_size feature to Drupal\migrate_drupal\Plugin\migrate\source\ContentEntity so it can scale.

$array = range(1, 4_000_000);

consumes 70MB. An array up to 6M crashes my (admittedly small) 128MB memory limit.

> To do this properly, we should provide incremental loading of ids directly in the entity query.
> We cannot add this to the existing query interface, but we can add an extended interface for this purpose.

What's the reason we can't add this to the query interface?

consumes 70MB. An array up to 6M crashes my (admittedly small) 128MB memory limit.

That's small indeed. And 6M is a lot. But both still in a realistic range.
So this shows we really need to load the ids in increments or in chunks.

What's the reason we can't add this to the query interface?

If we add a method to an interface, this breaks existing implementations in 3rd party code.
The BC-friendly way is to create a new interface extending the existing one, and make it optional to implement.

It could use SplQueue to get more optimized results

PS: also there https://pecl.php.net/package/ds notes https://medium.com/@rtheunissen/efficient-data-structures-for-php-7-9dda...

Here is a POC/preview branch with:
- Proposed changes to the current branch
- A new commit to add iterator functionality to the entity query, to iterate over ids.

https://git.drupalcode.org/issue/drupal-2577417/-/compare/11.x...2577417...
https://git.drupalcode.org/issue/drupal-2577417/-/compare/2577417-add-an...

We can have a look at this, but I think we should split out the entity query ids iterator to a separate issue.

A new commit to add iterator functionality to the entity query, to iterate over ids.

The test I added here is a big copy paste, where ->execute() is replaced with ->getIterator() + iterator_to_array().
We should do something more sophisticated to avoid the repetition. We should cover the iterator and array functionality with the same test code. For now I just wanted to prove that it works.

The preview PR turns the sql entity query into an IteratorAggregate.
IteratorAggregate is subtly implies a stateless object, where iterating over the values repeatedly produces the same result.
At first I was a bit skeptical if this is the case, but it seems that it is.

If we add a method to an interface, this breaks existing implementations in 3rd party code.

We have a base class in this case, so it's allowable in a minor release with a change record.

abstract class QueryBase implements QueryInterface 

Adding changes in MR as patch before rebasing to 11.x

Changed branch of the MR to 11.x as requested in #contribute by Bhanu951