I have a problem with update core from 8.6.x to 8.7.x. My site use default content type article with field_tags. Currently, I have about 600k term and when I can't finish drush updb because memory leak issue. When post update Taxonomy terms have been converted to be revisionable run about 90k this task using about 4G ram and still increase.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

doidd created an issue. See original summary.

doidd’s picture

Priority: Normal » Critical
doidd’s picture

@cilefen

  • I upgrade from 8.6 to 8.7.6 so not same with issue #3066439. (Code include commit https://git.drupalcode.org/project/drupal/commit/dd26473)
  • And when I run drush updb don't have any error, just memory increase from 96Mb to 4G and stop when out of memory. So not same issue #3052464

My Stack: docker, nginx, php 7.3, postgresql

doidd’s picture

catch’s picture

Status: Active » Needs review
Issue tags: +Drupal 8 upgrade path
catch’s picture

Issue tags: +WI critical
amateescu’s picture

Title: Memory leak when run converted to be revisionable » Memory leak in the entity schema converter
Component: taxonomy.module » entity system
Status: Needs review » Reviewed & tested by the community
Issue tags: -memory issues

@doidd, very nice find!

Manually tested this with ~13K terms by checking memory usage with and without the patch, and the results speak for themselves :)

HEAD: 600.7 MiB
Patch: 81.4 MiB

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlFieldableEntityTypeListenerTrait.php
@@ -220,6 +220,9 @@ protected function copyData(EntityTypeInterface $entity_type, EntityTypeInterfac
+    $temporary_storage->resetCache($entity_identifiers);

$entity_identifiers could be a list of ID or revision IDs, so we can't pass it directly to resetCache(). Instead, we should call resetCache() without any argument in order to clear everything.

Fixed in the patch attached.

amateescu’s picture

Oops, forgot to attach the patch.

catch’s picture

Status: Reviewed & tested by the community » Needs work

I think the patch as it stands will result in clearing the entire persistent entity cache each time, see #2635440: Document what cache clearing from ContentEntityStorageBase::resetCache() actually clears. However it should now be possible to empty entity.memory_cache directly - see https://www.drupal.org/node/2973262

Another issue that would help with this is #1199866: Add an in-memory LRU cache.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
786 bytes
828 bytes

@catch, I was thinking that a generic resetCache() call would also take care of the revision cache (when we'll have it), but that will probably use the same entity.memory_cache service so we should be fine with a more targeted approach. I tested this new patch in the same scenario as #8 and got the same results, the memory leak is fixed.

  • catch committed eee797f on 8.8.x
    Issue #3074949 by amateescu, doidd, catch: Memory leak in the entity...

  • catch committed 563c6b5 on 8.7.x
    Issue #3074949 by amateescu, doidd, catch: Memory leak in the entity...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Related issues: +#1199866: Add an in-memory LRU cache

Committed/pushed to 8.8.x, and cherry-picked to 8.7.x, thanks!

catch’s picture

double post

catch’s picture

Status: Fixed » Needs work

I just realised that #3055443: Switch to a null backend for all caches for running the database updates (which I just committed), renders this issue a bit moot. However I think we should probably leave it as is, since the code could potentially be used outside an update (like a drush command or similar), and because I didn't backport the other patch to 8.7

catch’s picture

Status: Needs work » Fixed

Didn't mean to change the status, leaving fixed.

Status: Fixed » Closed (fixed)

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