Large volume entity migrations run out of memory

We've had some discussion of this in IRC. Migrate (in 7 & 8) monitors memory usage and when reaching a threshold (defaulting to 85% of memory_limit) issues a drupal_static_reset() and checks to see if "enough" memory was released. This works just fine with Drupal 7 core - node caches and such use drupal_static() and get cleared, migrations generally only run into memory problems with the odd contrib or custom module doing local static caching.

In Drupal 8, however, EntityStorageBase::$entities contains an in-memory cache of entities. To clean them up, migration would need to find all the EntityStorageBase instances and call resetCache() on each of them. First, though, a question - is there any reason EntityStorageBase shouldn't be using drupal_static() to cache the entities?

Would be great to limit that clearing with only target entities from executed migrations

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -568,7 +568,17 @@ protected function attemptMemoryReclaim() {
+    foreach ($manager->getDefinitions() as $id => $definition) {
+      $manager->getStorage($id)->resetCache();

would be great to skip caches that have static disabled

it is resolving an excessively high memory event and we should try to clear every thing in Drupal

So why that does not fire event?
there's no way to other sub-systems to get involved here.

And \Drupal\Core\Entity\Sql\SqlContentEntityStorage::resetCache() implementation causes cache_tag invalidation, which could be very expensive.
Suppose we need follow-up here or at least document that operation could cause a chained invalidations

1. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -568,7 +568,17 @@ protected function attemptMemoryReclaim() {
   +      $manager->getStorage($id)->resetCache();
   

   I'm mostly worried because this calls Cache::invalidateTags(array($this->entityTypeId . '_values'));

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -568,7 +568,17 @@ protected function attemptMemoryReclaim() {
        // @TODO: explore resetting the container.
   

   Let's remove that todo or add issue to explore that

Is there a particular reason not to have the in-memory entity cache use drupal_static()? It seems that resetCache() is too heavy-handed, and using drupal_static() would make that in-memory cache work just like all other in-memory caches without undesirable side-effects.

I do feel that the entity cache needs proper static caching (specifically a means of clearing the static cache without invalidating external caches), and until #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset() drupal_static() is it.

All functions requiring a static variable to persist or cache data within a single page request are encouraged to use this function unless it is absolutely certain that the static variable will not need to be reset during the page request. By centralizing static variable storage through this function, other functions can rely on a consistent API for resetting any other function's static variables.

Entity caching certainly qualifies, and is (unless I'm missing something big somewhere else) the biggest static memory user.

So, attached is a patch to use drupal_static() for the static entity cache. It's pretty simple and should be perfectly transparent - the one known issue is it'll break unit tests, which don't include bootstrap.inc, so we'd need to "mock" drupal_static/drupal_static_reset there. Knowing that, I'll let the testbot take a crack just to be sure there aren't any simpletests broken by this.

So #2501117: Add static caching for PermissionsHashGenerator::generate() adds a cache.static bin using the memory backend. That addition is just a couple of lines if for some reason that patch doesn't get committed.

#1596472: Replace hard coded static cache of entities with cache backends is very outdated but I was trying to replace the static cache for entities with a cache backend - would need to start from scratch but that could be done with cache.static or cache.entity_static similar to the above. Scope could be a lot narrower than the issue was originally.

Then #1199866: Add an in-memory LRU cache is also outdated but was an LRU version of the memory backend with a limit on the number of items. Could either use a numeric limit on number of items, or just check peak usage in the backend each time there's a set and empty it at 60% like migrate does, or some kind of combination.

Then migrate could just ensure that the LRU / memory checking version of the cache backend is used in the container and might not need to do anything special at all otherwise. Or it can call MemoryBackend::reset() manually.

So, those are all good issues, but are any of them going to make 8.0? I have to admit the drupal_static() approach is also a longshot for 8.0 at this stage, but with no API changes seems like a possibility.

We need some way for migrations to make space for themselves today - of the various issues that involve improving entity caching, which has the best chance to get into 8.0? Whichever one that is, I'm happy to contribute to pushing it forward.

o, those are all good issues, but are any of them going to make 8.0?

They're all performance/scaling improvements - and in the case of OOM straight bug fixes, so they can go in up to and potentially during release candidate if someone works on them. There's no API change in any of these, only API additions which is fine during beta as long as there's a concrete use case that needs it.

The LRU cache backend I'd personally like available in core, but that could be done in contrib once entities are using the static cache backend/service instead of a raw object property - and migrate can then depend on that either way. Or as mentioned use the reset() method to clear the entire backend manually instead of LRU.

This sounds pretty bad, so escalating to major.

Definitely -1 on introducing drupal_static() in there. Using a memory backend with cache tag support (would also remove 95% of the cases where we need to reset static caches manually in tests) or other OO replacement, maybe, but not that.

Patch in #2 looks good to me as a quick fix for this issue. There are also long-standing issues about introducing a limit of how many nodes are kept in the static cache, that would be another approach on how this could be solved long-term.

Cache tag invalidation on a long running migration (or any sort of long running process) is flawed anyway due of the static cache that prevents repeated invalidations, so only the first node would actually invalidate the list cache tag. And fixing that to do it all the time wouldn't be fun either as that would most definitely bring a live site down ;)

The only thing that might work is doing the actual invalidations after the migration completed, but that has problems too.

Want to re-upload your patch so it's clear which one should be committed? I can RTBC then.

Also, issue summary should explain the solution/workaround here.

Cache tag invalidation on a long running migration (or any sort of long running process) is flawed anyway due of the static cache that prevents repeated invalidations, so only the first node would actually invalidate the list cache tag. And fixing that to do it all the time wouldn't be fun either as that would most definitely bring a live site down ;)

We could look at rate-limiting that rather than either once or every time - enforce once per configurable-amount-of-seconds per-request. Since it's just a static cache that's not too much extra work to implement.

Let me summarize how I understand the issue:

• Migration creates lots of entities (and other stuff) in a single PHP process (we'll talk about drush here, since a single batch through the UI isn't usually going to run into memory problems).
• Entities that are created are statically cached in the current process memory, in the $entities arrays in EntityStorageBase and ConfigEntityStorage.
• The migration process monitors memory usage, and when it gets to a certain threshold (default 85% of PHP's memory_limit) attempts to make room for itself by calling drupal_static_reset(). In D7, this was very effective - in most cases this would reclaim plenty of memory and we would seldom have to fallback to spawning a new drush process to continue the migration.
• Let's refer to the drupal_static() docs: "All functions requiring a static variable to persist or cache data within a single page request are encouraged to use this function unless it is absolutely certain that the static variable will not need to be reset during the page request. By centralizing static variable storage through this function, other functions can rely on a consistent API for resetting any other function's static variables."
• Nothing uses anywhere near the static memory that the entity static storage does, at least in the migration context, yet it does not use drupal_static() for the static memory cache.

Yes, there are a number of other things that can be done to improve static entity caching, but I still don't understand why whatever static caching is done for entities should be exempt from the documented purpose of drupal_static(). It's certainly poor DX to have a documented "consistent API for resetting any other function's static variables" and yet have to remember that entities are special and you have to call a different API to reset them...

It's certainly poor DX to have a documented "consistent API
for resetting any other function's static variables" and yet have to remember
that entities are special and you have to call a different API to reset
them...

The entity system is in no way special. In 7.x, we had 180 calls to drupal_static() according to api.drupal.org while in 8.x, we only have 50 left. And looking at the remaining calls, they are almost exclusively in old code.

That it frees memory is a side effect IMHO, the main use case for it was being able to reset the environment for tests. The way we do that in 8.x now is the container and dependency injection.

Resetting this could in some cases even have undesired effects, for example https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi..., resetting it might actually result in dropped information that can't be recovered. Just an example, deprecated and very likely unrelevant for processed that migrate, but it also was the very first one that I looked at when searching for such an example, there are likely others.

The ability to free unnecessary memory is a critical use case for migration, and drupal_static() is the only existing general mechanism for that.

As for resetting resulting in dropped information, anywhere that happens is a case of misusing drupal_static() - "...use this function unless it is absolutely certain that the static variable will not need to be reset during the page request".

Must migration have a laundry list of module- or class-specific API calls to make for reclaiming memory? And what about contrib modules - if drupal_static() is being (at least informally) deprecated, will we go back to the D6 days of static caches that can't be reset?

Tagging this as migrate critical. We could untag it later, but don't want to forget this one.

I haven't been involved in this issue, but reading over it I support using @neclimdul's patch in #2 as a quick fix, because this is a big deal. The larger discussion about how static caching is done for entities is very much worth having, but it's bigger than Migrate and should not block this fix. Once a more consistent static caching system is in core, this can and should be revisited. But until then, we need to be able to reclaim memory effectively.

I'm OK with this patch, so I vote RTBC. However, I'd like someone more familiar with the intricacies of the Entity API to review and RTBC it for real.

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -568,7 +568,17 @@ protected function attemptMemoryReclaim() {
+    // Entity storage can blow up with caches so clear them out.
+    $container = \Drupal::getContainer();
+    /** @var \Drupal\Core\Entity\EntityManagerInterface $manager */
+    $manager = $container->get('entity.manager');

There's \Drupal::entityManager() for that

Note that this will also invalidate the persistent cache, but running migrations is going to be a severe hit on your performance anyway so this is probably acceptable, at least as a temporary solution.

I had a quick look into just resetting the static caches, we don't really have an API for that, we could reset the handlers property in the entity manager with useCaches(FALSE); useCaches(TRUE), but they might still be injected somewhere and not really freed.

Seems like everyone is on board with this being a reasonable quick fix, and it's unclear whether more expansive fixes are possible. That's a good argument for getting this in, possibly with a follow-up issue to dig in more.

I inquired about test coverage but it's not easy to test memory consumption.

Committed and pushed to 8.0.x. Thanks!

#32 sounds like a good novice issue, but don't want to hold up a migrate critical on that.

Opened #2558857: Migrations invalidate entity caches when trying to reclaim memory, should flush to track the persistent cache clearing.

There are two cases that can be tricky:

- migrations that need to load entities as part of the migration may slow down a lot - some sites take 10 or 20 hours to migrate so a 10% regression can mean an extra hour or two.
- partial migrations on a live site

Follow-up (quickfix) for #32
#2559191: Clean-up migrate executable

Still seeing the migration run out of memory... #2688297: File migration slows down and eats more and more memory, eventually stops