Remove memory management from MigrateExecutable

1. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -470,35 +455,29 @@ protected function checkStatus() {
   +            '@pct' => round($this->memoryManager->getUsageRatio() * 100),
   ...
   +              '@pct' => round($this->memoryManager->getUsageRatio() * 100),
   
   @@ -510,9 +489,9 @@ protected function memoryExceeded() {
   +              '@pct' => round($this->memoryManager->getUsageRatio() * 100),
   

   This too. A constant on the memory manager.

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -470,35 +455,29 @@ protected function checkStatus() {
   +      if ($this->memoryManager->reclaim()->isLimitExceeded(0.90)) {
   

   Can we move this magic number into a constant?

This moves more things into the memory manager.

phenaproxima asked me to look at fixing the failing test in #4 and I didn't get it done and heddn beat me to it anyway. Nice to see the tests fixed but I think that adding all the messaging clutters the MemoryManager and makes it harder to concentrate on what it is doing. I've always been a fan of doing one thing well and would prefer to see that done here.

How about a follow up to deal with the messaging?

There's nothing wrong with this code per se, but we'd like to remove MigrateExecutable entirely before Drupal 9.
Really? How do we expect migrate to work without it? I can barely get some migrations to run _with_ it and generally have to write weird hacks into hooks to get drupal's long process memory usage under control. I don't think this is realistic.

I _do_ think moving it to a class has a lot of benefits though. There are actually problems as I hinted at with the code like there's no way to tune it, effectively use migrations it if memory_limit was set to '-1', or if you have other logic you need to implement.

Moving it to a class would let it be pushed on the container, inject tuneables like limits and thresholds, and be replaced.

Reading your response and the additional clarity from slack I think I understand. Marking the memory management internal and discussions of killing the Executable I miss-interpreted that as expecting to kill the memory management in 9.

I think making it internal and not making an interface misses a big opportunity. For the past 3 years I've struggled with Drupal's caches and migrate's memory management. Its _almost_ bullet proof now but the arbitrary thresholds and assumptions around things like memory_limit=-1 can force some really weird site builder compromises... Like the hook_row_alter I had to to do futze with clearly caches when Migrate's didn't work for me.

Making this a service that can be replaced or tune would be a really cool developer win. So much so I implemented it and am applying it and using it instead of my hook_row_alter hackery! :-D

On the messaging discussion, that's actually the documented logic from checkStatus(). Since there are lots of messages on reclaim successes and they're all related to internal values of the class I don't really see a way to do it other than in the memory managment service.

Something I've seen done with several places in core and contrib is to fire an event when certain things happen. What if we fired an event when we decide we need to reclaim memory? Then subscribers can decide to actually reclaim memory, log errors, dial home and/or do some fancy thing to drop memory even /more/ than what comes with core?

I didn't update tests, so this will fail. But this at least gives us an idea of what moving things into an event subscriber could look like.

Triaging issue queue.

Reroll. Fixed a missing quote in a/core/modules/migrate/migrate.services.yml which will reduce the test failures.

Coding standard fixes.

Changed the unit test to use a dataprovider and allow for a reclaim of memory.

Uploading the patch is always a good idea.

Quick review

1. +++ b/core/modules/migrate/migrate.services.yml
   @@ -36,3 +36,11 @@ services:
   +  migrate.memory_manager:
   +    class: Drupal\migrate\MemoryManager
   +    arguments: ['@event_dispatcher', '0.9', '0.85']
   

   Would be great to have a comment why this settings default, probably in CR

2. +++ b/core/modules/migrate/src/Event/MigrateMemoryLimitEvent.php
   @@ -0,0 +1,95 @@
   +   * The memory usage ratio.
   +   *
   +   * @var float|int
   +   */
   +  protected $usageRatio;
   

   why it can be int?

3. +++ b/core/modules/migrate/src/EventSubscriber/MemoryLimitExceeded.php
   @@ -0,0 +1,126 @@
   +class MemoryLimitExceeded implements EventSubscriberInterface {
   ...
   +  use StringTranslationTrait;
   ...
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
   

   better to inject string_translation service

4. +++ b/core/modules/migrate/src/EventSubscriber/MemoryLimitExceeded.php
   @@ -0,0 +1,126 @@
   +    if ($event->getPhase() != 'pre reclaimed') {
   ...
   +  public function notify(MigrateMemoryLimitEvent $event) {
   ...
   +    switch ($event->getPhase()) {
   +      case 'pre reclaimed':
   ...
   +      case 'still exceeded':
   ...
   +      case '':
   
   +++ b/core/modules/migrate/src/MemoryManager.php
   @@ -0,0 +1,143 @@
   +      $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), 'pre reclaimed');
   +      $this->dispatcher->dispatch(MigrateEvents::MEMORY_LIMIT, $event);
   ...
   +      if ($this->isLimitExceeded($this->memoryReclaimThreshold)) {
   +        $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), 'still exceeded');
   +        $this->dispatcher->dispatch(MigrateEvents::MEMORY_LIMIT, $event);
   ...
   +      $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), 'reduced enough to continue');
   +      $this->dispatcher->dispatch(MigrateEvents::MEMORY_LIMIT, $event);
   

   this strings could live in manager interface

5. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -99,22 +91,17 @@ class MigrateExecutable implements MigrateExecutableInterface {
   -  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message = NULL, EventDispatcherInterface $event_dispatcher = NULL) {
   +  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message = NULL, EventDispatcherInterface $event_dispatcher = NULL, MemoryManagerInterface $memory_manager = NULL) {
   ...
   +    $this->memoryManager = $memory_manager;
   
   @@ -145,6 +132,18 @@ protected function getEventDispatcher() {
   +  protected function getMemoryManager() {
   +    if (!$this->memoryManager) {
   +      $this->memoryManager = \Drupal::service('migrate.memory_manager');
   +    }
   +    return $this->memoryManager;
   
   @@ -455,121 +454,10 @@ protected function handleException(\Exception $exception, $save = TRUE) {
   -    if ($this->memoryExceeded()) {
   +    if (!$this->getMemoryManager()->ensureMemory()) {
   

   It needs deprecation fallback in constructor, the new method used only once

#1. Added an item in the IS so this doesn't get forgotten.
#2. I think this is copy/paste and since int doesn't make much sense for a ratio, all occurrences are now float.
#3. Done
#4. Added strings to the interface. Maybe better names are needed but nothing coming to mind today.
#5. Not sure what is expected here. What is to deprecated and is there a problem with a new method being used only once?

The patch includes code standard fixes as well.

I wonder if anybody would be interested in getting #3000050: Replace \Drupal\Component\Utility\Environment::checkMemoryLimit with a PHP memory checking helper class and service in, in order to have a common memory checking component. I have been trying to improve memory managemnt in the context of GD image toolkit since #2583041: GD toolkit & operations should catch \Throwable to fail gracefully in case of errors, but without success so far.

It looks mostly ready! Thanks a lot moving forward 👍

Meantime @TODO: explore resetting the container. needs follow-up link to pass

Also "memory checker" should be memory manager as it more common, and could care about GC used right after the todo

NW for #32.

32. Made a new issue for the long standing @TODO, #3128793: Explore resetting the container when reclaiming memory

What is 'memory checker' referring to? A `grep -ri 'memory checker' core` returned nothing, so no change made to this patch.

#25.5 is okay. Nothing is deprecated and the argument is optional, the class uses a getMemoryManager() method which will get the service if it isn't passed in. It's all a little wonky, but MigrateExecutable isn't a service, so we can't force the dependency injection easily. We could deprecate NOT passing the memory manager service to the executable, but that would require changing the order of constructor arguments, and if the goal is to deprecate the executable entirely in D9 (and I still think it should be) then there isn't really a point.

I think the patch is good. I'd RTBC it, but I'm setting to NW for the missing CR in the IS.

CR added

The CR look fine to me. There is just one thing, the IS has a task for the CR, "Explain why the threshold is 0.9 and the memory limit is 0.85". I would update the CR but I don't know why those values where chosen, only that they are the same as the existing values.

I was wondering if there was more information in the original commit of MigrateExecutable about the threshold values. It was commited in #2125717: Migrate in core: patch #1 and I didn't find any history of those values there.

And, the request for adding more information about the threshold is from #29.1

CR updated and minor tweak to use constants rather then magic strings.

+++ b/core/modules/migrate/src/MemoryManager.php
@@ -77,12 +77,12 @@ public function __construct(EventDispatcherInterface $dispatcher, $reclaim_thres
-      $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), 'pre reclaimed');
+      $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), MemoryManagerInterface::PRE_RECLAIMED);
...
-        $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), 'still exceeded');
+        $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), MemoryManagerInterface::STILL_EXCEEDED);

maybe better to use self::CONSTANT as class implements the interface by itself

Replaced string with the constant REDUCED_ENOUGH_TO_CONTINUE.

I think the CR needs another tweak "But if after reclamation at least 5% or (or less then 85%) is reached," is a bit confusing but I'm not sure about the 5%.

CR updated.

I have started to look at this issue. I may have more requests as I continue.

1.   +++ b/core/modules/migrate/src/MemoryManager.php
     @@ -0,0 +1,143 @@
     +   * @param int $memory_limit
     +   *   The memory limit.
     +   */
     +  public function __construct(EventDispatcherInterface $dispatcher, $reclaim_threshold, $memory_threshold, $memory_limit = NULL) {
     +    $this->dispatcher = $dispatcher;
     +    $this->memoryReclaimThreshold = $reclaim_threshold;
     +    $this->memoryThreshold = $memory_threshold;
     +    // Record the memory limit in bytes.
     +    if (isset($memory_limit)) {
     +      $this->memoryLimit = Bytes::toInt($memory_limit);
     +    }
     +    else {
     +      // Auto detect memory limit.
     +      $limit = trim(ini_get('memory_limit'));
     +      if ($limit == '-1') {
     +        $this->memoryLimit = PHP_INT_MAX;
     +      }
     +      else {
     +        $this->memoryLimit = Bytes::toInt($limit);
     +      }
     +    }
     +  }

   If the parameter $memory_limit is an integer, then we do not need to apply Bytes::toInt() to it. If we want to allow strings, like 128M, then change the @param comment.

2. The remaining usage of Bytes::toInt() should be cast to int, with a note to remove the cast once #3142934: Replace \Drupal\Component\Utility\Bytes::toInt() with \Drupal\Component\Utility\Bytes::toNumber() due to return type goes in. Alternatively, we could just make it a float (here and in the rest of the patch).

3. What do we want to do if $memory_limit is supplied and equal to -1? Depending on the answer to that, and to (1) above, this might be simpler:

    if (empty($memory_limit)) {
      // Auto detect memory limit.
      $memory_limit = trim(ini_get('memory_limit'));
    }
    $this->memoryLimit = ($memory_limit == -1)
      ? PHP_INT_MAX
      : Bytes::toInt($memory_limit);

4.   +  public function ensureMemory() {
     +    if ($this->isLimitExceeded()) {
     +      $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), self::PRE_RECLAIMED);
     +      $this->dispatcher->dispatch(MigrateEvents::MEMORY_LIMIT, $event);
     +      // Re-check the reclaim threshold to ensure we reclaimed enough to
     +      // continue.
     +      if ($this->isLimitExceeded($this->memoryReclaimThreshold)) {
     +        $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), self::STILL_EXCEEDED);
     +        $this->dispatcher->dispatch(MigrateEvents::MEMORY_LIMIT, $event);
     +        return FALSE;
     +      }
     +      $event = new MigrateMemoryLimitEvent($this->getUsageRatio(), $this->getUsageInBytes(), $this->getLimit(), self::REDUCED_ENOUGH_TO_CONTINUE);
     +      $this->dispatcher->dispatch(MigrateEvents::MEMORY_LIMIT, $event);
     +    }
     +    return TRUE;
     +  }

   In three places, we create and dispatch an event, and the only difference is the "phase" constant. I think that is enough to justify adding a helper function.

   If the function is rewritten to exit early, then it avoids indenting most of the function body at the expense of a second return TRUE line.

5.   +++ b/core/modules/migrate/src/MemoryManagerInterface.php
     @@ -0,0 +1,33 @@
     +  /**
     +   * Tests whether we've exceeded the desired memory threshold.
     +   *
     +   * @return bool
     +   *   TRUE if the threshold is exceeded, otherwise FALSE.
     +   */
     +  public function ensureMemory();

   This function does more than just test. It also dispatches up to two events. Can we add a sentence or two to this doc block to say what it really does?

6.   +   * @param int $multiplier
     +   *   (optional) A percentage of the threshold. If given, we will consider the
     +   *   memory limit exceeded if the current memory usage is over this percentage
     +   *   of the threshold. Defaults to 1 (i.e., the threshold is unchanged).
     +   *
     +   * @return bool
     +   *   TRUE if the memory limit is exceeded, otherwise FALSE.
     +   */
     +  protected function isLimitExceeded($multiplier = 1) {

   Make it @param float.

7. Do we need this wrapper function? Is the idea that child classes may override it?

    +  public function getUsageInBytes() {
    +    return $this->usageInBytes;
    +  }

8. Maybe this answers the previous question:

    +++ b/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php
    @@ -66,40 +68,56 @@ public function handleException(\Exception $exception, $save = TRUE) {
    ...
    +  public function getUsageInBytes() {
    +    return $this->memoryUsage;

   Is there any reason to make the test method public?

9. Add "in bytes" in a few places:

    +++ b/core/modules/migrate/src/Event/MigrateMemoryLimitEvent.php
    @@ -0,0 +1,99 @@
    ...
    +  /**
    +   * The memory limit.
    +   *
    +   * @var int
    +   */
    +  protected $limit;
    ...
    +   * @param int $usage_in_bytes
    +   *   The memory usage.
    +   * @param int $limit
    +   *   The memory usage limit in bytes.
    +   * @param string $phase
    +   *   The phase of memory reclamation.
    +   */
    +  public function __construct($usage_ratio, $usage_in_bytes, $limit, $phase) {
    ...
    +  /**
    +   * Gets the memory limit.
    +   *
    +   * @return int
    +   *   The memory limit.
    +   */
    +  public function getLimit() {

10.   +  /**
      +   * The phase of memory reclamation.
      +   *
      +   * @var string
      +   */
      +  protected $phase;

    Can we say that the phase is one of the constants defined in MemoryManagerInterface? I think that should go here, but maybe on the getter function or both.

11. Make it "an action", not "and action".

     +++ b/core/modules/migrate/src/Event/MigrateEvents.php
     @@ -181,4 +181,19 @@ final class MigrateEvents {
     ...
     +  /**
     +   * Name of the event fired when the memory limit is reached.
     +   *
     +   * This event allows modules to perform and action when the limit is reached.

12.   +++ b/core/modules/migrate/src/EventSubscriber/MemoryLimitExceeded.php
      @@ -0,0 +1,151 @@
      +  public function notify(MigrateMemoryLimitEvent $event) {
      +    $ratio_multiplier = 100;
      +    switch ($event->getPhase()) {
      +      case MemoryManagerInterface::PRE_RECLAIMED:
      +        $this->message->display($this->t(
      +          'Memory usage is @usage (@pct% of limit @limit), reclaiming memory.',
      +          [
      +            '@pct' => round($event->getUsageRatio() * $ratio_multiplier),
      +            '@usage' => $this->formatSize($event->getUsageInBytes()),
      +            '@limit' => $this->formatSize($event->getLimit()),
      +          ]
      +        ), 'warning');
      +        break;
      +
      +      case MemoryManagerInterface::STILL_EXCEEDED:
      +        $this->message->display($this->t(
      +          'Memory usage is now @usage (@pct% of limit @limit), not enough reclaimed, starting new batch',
      +          [
      +            '@pct' => round($event->getUsageRatio() * $ratio_multiplier),
      +            '@usage' => $this->formatSize($event->getUsageInBytes()),
      +            '@limit' => $this->formatSize($event->getLimit()),
      +          ]
      +        ), 'warning');
      +        break;
      +
      +      case MemoryManagerInterface::REDUCED_ENOUGH_TO_CONTINUE:
      +      case '':
      +        $this->message->display($this->t(
      +          'Memory usage is now @usage (@pct% of limit @limit), reclaimed enough, continuing',
      +          [
      +            '@pct' => round($event->getUsageRatio() * $ratio_multiplier),
      +            '@usage' => $this->formatSize($event->getUsageInBytes()),
      +            '@limit' => $this->formatSize($event->getLimit()),
      +          ]
      +        ));
      +        break;
      +    }
      +  }

    DRY. Define the parameters once, outside the switch statement. Just use 100 inside round(): there is no need for the $ratio_multiplier variable.

13.   +  protected function formatSize($size) {
      +    return format_size($size);
      +  }

    Do we need this wrapper function?

Fixes for #46;
1. Update the @param
2,3. Seems reasonable to all ow -1 so used the simpler option in #3.
4. Helper added as well as return early.
5. Added some comments. Doesn't seem like enough, more like a start.
6. Fixed
9. 'in bytes' liberally applied.
11. Fixed
12. Fixed
13. Doesn't look like it is needed. Removed.

TODO
#46. 7, 8, 10

The failure in #47 appears to be random, not related to this issue.

7,8. I don't know if it should be protected or public.
10. Not sure here either. Decided to ass a comment to the declaration of $phase, not the getter.

I still have a few suggestions based on the patch from #43. I will look at the latest patch on my next review, and I still have to review the tests.

1. I already mentioned "and action", but I missed that "event lister" should be "event listener".

    +++ b/core/modules/migrate/src/Event/MigrateEvents.php
    @@ -181,4 +181,19 @@ final class MigrateEvents {
    +  /**
    +   * Name of the event fired when the memory limit is reached.
    +   *
    +   * This event allows modules to perform and action when the limit is reached.
    +   * The event lister method receives a
    +   * \Drupal\migrate\Event\MigrateMemoryLimitEvent.

2. The getter returns whatever was passed to the constructor, so we should use the interface for the @return comment.

    +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -144,6 +130,19 @@ protected function getEventDispatcher() {
    +  /**
    +   * Gets the migration memory manager.
    +   *
    +   * @return \Drupal\migrate\MemoryManager
    +   *   The migration memory manager.
    +   */
    +  protected function getMemoryManager() {

3.   @@ -455,121 +454,10 @@ protected function handleException(\Exception $exception, $save = TRUE) {
     -    if ($this->memoryExceeded()) {
     +    if (!$this->getMemoryManager()->ensureMemory()) {
            return MigrationInterface::RESULT_INCOMPLETE;
          }
          return MigrationInterface::RESULT_COMPLETED;
        }

   I do not see any discussion of why we are replacing memoryExceeded() (returning TRUE if we are out of memory) with ensureMemory() (returning FALSE if we are out of memory). Maybe #46.5 is the reason for changing the name: the function does more than just test.

   If we are going to negate the meaning of the return value, then I would rather swap the two return statements than add an explicit negation (!). Better yet:

     return $this->getMemoryManager()->ensureMemory()
       ? MigrationInterface::RESULT_COMPLETED
       : MigrationInterface::RESULT_INCOMPLETE;

Fixes for #51, 1 , 2 and 3.

It looks like ensureMemory() was added in #13.

The patch in #49 still says "event lister" (#50.1). Maybe you uploaded the wrong patch or had unsaved/uncommitted changes?

Sorry about that. This fixes the spelling of 'listener'.

Thanks for the updates. I still owe you a review of the change to the tests, but here are some comments on the patch in #53:

1. This is a good start:

    +++ b/core/modules/migrate/src/MemoryManagerInterface.php
    @@ -0,0 +1,36 @@
    +   * @return bool
    +   *   TRUE if the threshold is exceeded, otherwise FALSE. If memory usage has
    +   *   exceeded the limit then a PRE_RECLAIMED MigrateEvent is dispatched, This
    +   *   is followed by either a STILL_EXCEEDED MigrateEvent or a
    +   *   REDUCED_ENOUGH_TO_CONTINUE MigrateEvent if enough memory was reclaimed.
    +   */
    +  public function ensureMemory();

   But I think we should rewrite the first sentence. In fact, it is worse than that: the first sentence is backwards! Compare #50.3. How about something like this before the @return comment

   If memory usage is too high, then dispatch a PRE_RECLAIMED MigrateEvent so that event listeners can reclaim memory. Then dispatch a STILL_EXCEEDED MigrateEvent or a REDUCED_ENOUGH_TO_CONTINUE MigrateEvent, depending on whether enough memory was reclaimed.

   and then the @return comment can be shorter, like

   TRUE if memory usage is low enough, or if enough memory can be reclaimed. FALSE if memory usage is too high and not enough can be reclaimed.

2. I still think that this should be protected (#46.8):

    +++ b/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php
    @@ -66,40 +68,56 @@ public function handleException(\Exception $exception, $save = TRUE) {
    +  public function getUsageInBytes() {
    +    return $this->memoryUsage;

   I see the problem. I quoted the wrong lines of code in #46.7. There are two methods named getUsageInBytes(): one on the event class, which is public, and one declared in MemoryManagerInterface, implemented in MemoryManager, and overridden in TestMemoryManager. The second one is declared and implemented as protected, but overridden as public.

3. In #46.12, I meant to suggest defining the parameters as a single array (before the switch statement), not three separate variables. For the record, in reviewing earlier comments I noticed that #3 explicitly requests replacing 100 with a constant. I disagree: this does not count as a "magic number" in the context of formatting a per cent. Maybe #3.1, like my #46.7, made the mistake of quoting the wrong lines. That is, maybe the intention was to point out the 0.85.

4. The @return comment for MigrateExecutable::getMemoryManager() still says MemoryManager instead of MemoryManagerInterface (#50.2).

1. Fixed
2. Not following this yet. getUsageInBytes() in not declared on MemoryManagerInterface it is only in MemoryManager. Similarly, IsLimitExceeded is protected in MemoryManager and public in TestMemoryManager. Whats the difference?
3. Yep, I planned to do that and completely forgot. Fixed now.
4. Argh! Fixed, again.

I seem to be jinxed on getUsageInBytes()! Of course you are right: if it were declared in the interface, then it would be public.

The same question applies to both getUsageInBytes() and isLimitExceeded(): if they are protected in MemoryManager, then why make them public in TestMemoryManager?

I see that isLimitExceeded() is called by the main test class, so it has to be public: that answers my question. I do not see an answer for getUsageInBytes().

I will not look at the new patch yet. Instead, I will finally review the changes to the tests,

Here is a review of the changes to the tests.

1. Switching the order of the parameters $memory_exceeded and $memory_limit makes the patch harder to review. Is there a good reason for this switch?

        * @param string $message
        *   The second message to assert.
    -   * @param bool $memory_exceeded
    -   *   Whether to test the memory exceeded case.
    +   * @param int|null $memory_limit
    +   *   (optional) The memory limit. Defaults to NULL.
    +   * @param float $reclaim_threshold
    +   *   The ratio of the memory limit which will trigger a failed reclaim.
    +   * @param float $memory_threshold
    +   *   The ratio of the memory limit at which an operation will be interrupted.
        * @param int|null $memory_usage_first
        *   (optional) The first memory usage value. Defaults to NULL.
        * @param int|null $memory_usage_second
        *   (optional) The fake amount of memory usage reported after memory reclaim.
        *   Defaults to NULL.
    -   * @param int|null $memory_limit
    -   *   (optional) The memory limit. Defaults to NULL.
    +   * @param bool $memory_exceeded
    +   *   Whether to test the memory exceeded case.
    +   *
    +   * @dataProvider memoryExceededProvider
    +   *
    +   * @throws \Drupal\migrate\MigrateException
        */
    -  protected function runMemoryExceededTest($message, $memory_exceeded, $memory_usage_first = NULL, $memory_usage_second = NULL, $memory_limit = NULL) {

   Just a guess: earlier versions of the patch made $memory_limit required but kept other options optional, maybe even made $memory_exceeded optional. That would be a good reason to rearrange the parameters. In the current version, all parameters are required, so this reason no longer applies.

2. Maybe I am confused, but the MigrateExecutableMemoryExceededTest class seems to be all wrong.

    +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableMemoryExceededTest.php
    @@ -9,6 +9,13 @@
     <?php
   
     namespace Drupal\Tests\migrate\Unit;
   
     /**
      * Tests the \Drupal\migrate\MigrateExecutable::memoryExceeded() method.
      *
      * @group migrate
      */
     class MigrateExecutableMemoryExceededTest extends MigrateTestCase {

   (More context than the patch supplies.) See #50.3: this patch replaces memoryExceeded() (on the executable) with ensureMemory() (on the memory manager). Yet I still see memoryExceeded() here and in many other comments.

    @@ -53,70 +60,85 @@ protected function setUp(): void {
    -    $this->executable = new TestMigrateExecutable($this->migration, $this->message);
    -    $this->executable->setStringTranslation($this->getStringTranslationStub());

   Minor point: is there a good reason to move these lines from the setUp() method to the test method? The move makes the patch a little harder to review.

    -   * Runs the actual test.
    +   * Test the memory manager.

   Are we actually testing the memory manager?

    +    if ($memory_usage_second) {
    +      $memory_manager->reclaim();
    +    }
    +    $result = $memory_manager->isLimitExceeded(1);
    +    $this->assertEquals($memory_exceeded, $result);

   It looks as though we are actually testing the reclaim() method, which is defined in the test memory manager, not the real one. Maybe we are also testing isLimitExceeded(), which is made public in the test class so that it can be accessed here.

   I think this test class needs to be completely rewritten. If it is actually a unit test for the memory manager, then it should test the memory manager directly instead of working through the executable. It should certainly test ensureMemory(), the public method on the class. And it should probably be renamed.

3.   +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
     @@ -36,6 +36,13 @@ class MigrateExecutableTest extends MigrateTestCase {
     +  /**
     +   * The mocked memory manager.
     +   *
     +   * @var \Drupal\migrate\MigrateMessageInterface|\PHPUnit\Framework\MockObject\MockObject
     +   */
     +  protected $memoryManager;

   Fix the @var comment.

4.   @@ -119,6 +127,8 @@ public function testImportWithValidRow() {
     +    $this->memoryManager->method('ensureMemory')
     +      ->willReturn(TRUE);
          $this->assertSame(MigrationInterface::RESULT_COMPLETED, $this->executable->import());

   This makes sense: the import() method calls checkStatus(), which calls ensureMemory(), and for this test we want that to return TRUE. I am not sure why the existing test does not have something similar.

   As long as we are looking at these lines, add a blank line before the assertion, for consistency with the other test cases.

5.   +++ b/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php
     @@ -11,27 +15,25 @@
     ...
     +  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message = NULL, EventDispatcherInterface $event_dispatcher = NULL, MemoryManagerInterface $memory_manager = NULL) {
     +    parent::__construct($migration, $message, $event_dispatcher);
     +    $this->memoryManager = $memory_manager;
     +    if (!$memory_manager) {
     +      $this->memoryManager = new TestMemoryManager($this->eventDispatcher, NULL, NULL, NULL);
     +    }

   The TestMigrateExecutable class is used twice. In MigrateExecutableTest, the constructor is passed the optional $memory_manager argument, so we do not need to override the constructor. If we turn MigrateExecutableMemoryExceededTest into a unit test for the memory manager, as I suggested in (2), then we can get rid of this overridden constructor, along with several use statements.

   If we still need this, then I think it would be simpler to do something like this:

         $memory_manager = $memory_manager ?: new TestMemoryManager($this->eventDispatcher, NULL, NULL, NULL);
         parent::__construct($migration, $message, $event_dispatcher, $memory_manager);

   You might even combine that into one statement. Another option would be to override getMemoryManager() instead of overriding the constructor.

Just the simpler ones from #57. Fixes for #57.1, 3 and 4.

Also fix the @param comments: I mentioned in #57.1 that none of them are optional (from the function signature) but some of the comments still say "(optional)".

The testbot says there are some coding standards problems.

@benjifisher, thanks for getting me to redo the tests.

This patch adds testing for the new MemoryManager in MemoryMangerTest and removes MigrateExecutableMemoryExceededTest, since that test is now in the new MemoryManagerTest. Be aware that the test cases in MemoryManagerTest are not necessarily thorough, they were enough to get the tests setup.

This has been hard, but I think we are getting to the cleanup stage.

1. The testbot reports two coding standards problems (really one problem triggering two notices).

2.   +++ b/core/modules/migrate/src/EventSubscriber/MemoryLimitExceeded.php
     @@ -0,0 +1,124 @@
     +  public function notify(MigrateMemoryLimitEvent $event) {
     +    $results = [
     +      '@pct' => round($event->getUsageRatio() * 100),
     +      '@usage' => format_size($event->getUsageInBytes()),
     +      '@limit' => format_size($event->getLimit()),
     +    ];
     +    switch ($event->getPhase()) {
     +      case MemoryManagerInterface::PRE_RECLAIMED:
     +        $this->message->display($this->t(
     +          'Memory usage is @usage (@pct% of limit @limit), reclaiming memory.', $results), 'warning');
     +        break;

   The coding standards are not explicit on how to break long function calls, but the usual practice is to put the closing parentheses on a new line. Here, and in the other two cases, I prefer each argument to t() (literal string and $results) on a separate line, then one more line starting with the closing parenthesis.

3. The last part of #50.3 is still undone.

4. I am still curious about #57.4. Do you know why the previous version of the test worked without anything equivalent to $this->memoryManager->method('ensureMemory')->willReturn(TRUE);?

5.   +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
     @@ -36,6 +36,13 @@ class MigrateExecutableTest extends MigrateTestCase {
         */
        protected $executable;
   
     +  /**
     +   * The mocked memory manager.
     +   *
     +   * @var \Drupal\migrate\MemoryManagerInterface|\PHPUnit\Framework\MockObject\MockObject
     +   */
     +  protected $memoryManager;

   IIUC, we are moving away from MockObject and using Prophecy in new tests, so maybe it is not worth worrying about this. But isn’t the idea that when you create a mock object, it implements the given interface? If so, should we just use MemoryManagerInterface for the @var comment?

6. Out of scope:

    +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
    @@ -467,6 +493,19 @@ public function testProcessRowEmptyDestination() {
         $this->assertSame(['test2'], $row->getEmptyDestinationProperties());
       }
   
    +  /**
    +   * Tests the checkStatus method.
    +   */
    +  public function testCheckStatus() {
    +    $this->memoryManager->expects($this->any())
    +      ->method('ensureMemory')
    +      ->will($this->onConsecutiveCalls(TRUE, FALSE));
    +    $result = $this->executable->checkStatus();
    +    $this->assertEquals($result, MigrationInterface::RESULT_COMPLETED);
    +    $result = $this->executable->checkStatus();
    +    $this->assertEquals($result, MigrationInterface::RESULT_INCOMPLETE);
    +  }

   I am already tired, and I do not want to think about one more test if I do not have to! If this missing test coverage bothers you, then open a new issue for it.

7.   +++ b/core/modules/migrate/src/MigrateExecutable.php
     @@ -99,22 +91,15 @@ class MigrateExecutable implements MigrateExecutableInterface {
     -   * @throws \Drupal\migrate\MigrateException
     +   * @param \Drupal\Migrate\MemoryManagerInterface $memory_manager
     +   *   (optional) The memory manager.
         */
     -  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message = NULL, EventDispatcherInterface $event_dispatcher = NULL) {
     +  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message = NULL, EventDispatcherInterface $event_dispatcher = NULL, MemoryManagerInterface $memory_manager = NULL) {

   Removing the @throw comment is also out of scope, but I will allow it if you are sure that it is correct. I guess you trust PHPStorm? The only line that looks at all suspect is the call to getIdMap().

8. We got rid of this function in the MigrateExecutable class, so I think we can get rid of it here, too:

    +++ b/core/modules/migrate/tests/src/Unit/TestMigrateExecutable.php
    @@ -65,68 +51,24 @@ public function handleException(\Exception $exception, $save = TRUE) {
    +  protected function formatSize($size) {
    +    return $size;

9. This is not used anywhere:

    +  public function getMemoryManager() {
    +    return parent::getMemoryManager();

10. We do not need this unless I am wrong about (6) above:

     +  public function checkStatus() {
     +    return parent::checkStatus();

11. I have a few points so make about this:

     +++ b/core/modules/migrate/tests/src/Unit/MemoryManagerTest.php
     @@ -0,0 +1,300 @@
     ...
     +  /**
     +   * Tests ensureMemory.
     +   *
     +   * @param float $reclaim_threshold
     +   *   The ratio of the memory limit which will trigger a failed reclaim.
     +   * @param float $memory_threshold
     +   *   The ratio of the memory limit at which an operation will be interrupted.
     +   * @param int|null $memory_limit
     +   *   The memory limit. Defaults to NULL.
     +   * @param int|null $memory_usage
     +   *   The first memory usage value. Defaults to NULL.
     +   * @param int|null $cleared_memory_usage
     +   *   The fake amount of memory usage reported after memory reclaim.
     +   *   Defaults to NULL.
     +   * @param bool $expected_ensure_memory
     +   *   Indicated that the desired memory threshold was exceeded.
     +   *
     +   * @dataProvider providerTestEnsureMemory
     +   */
     +  public function testEnsureMemory($reclaim_threshold, $memory_threshold, $memory_limit, $memory_usage, $cleared_memory_usage, $expected_ensure_memory) {
     +    $memory_manager = new TestMemoryManager($this->eventDispatcher, $reclaim_threshold, $memory_threshold, $memory_limit);
     +    $memory_manager->setMemoryUsage($memory_usage, $cleared_memory_usage);
     +    if ($cleared_memory_usage) {
     +      $memory_manager->reclaim();
     +    }
     +    $result = $memory_manager->ensureMemory();
     +    $this->assertEquals($expected_ensure_memory, $result);
     +  }

    The function signature shows all parameters as required, but several of the @param comments mention defaults. Several other doc blocks have the same problem.

12. Instead of calling the reclaim() method, which is only defined on TestMemoryManager, I would like to see this happen when the ensureMemory() method dispatches the PRE_RECLAIMED event. The usual way to do this would be through the mocked event dispatcher, but it would be simpler to do it in the overridden dispatch() method.

13. Can we add a test case where ensureMemory() returns FALSE?

14. Several of these methods are just public wrappers for their parents. I guess the point is that we want to add tests for them, so they have to be accessible to the test class. But dispatchEvent() is not tested, so we only need to override it if we want to use it when testing ensureMemory(), as I suggested above. Even then, we do not need to make it public.

    Isn’t the usual practice to test only the public methods of the class, and let them call the protected methods?

     +class TestMemoryManager extends MemoryManager {
     ...
     +  /**
     +   * {@inheritdoc}
     +   */
     +  public function dispatchEvent($phase) {
     +    return parent::dispatchEvent($phase);
     +  }

15. The testGetUsageInBytes() test method tests the overridden getUsageInBytes() in TestMemoryManager. I do not think think that is useful.

16. This name is odd, but at least it is used consistently:

     +  /**
     +   * Provides data for testGetUsageRatio.
     +   */
     +  public function providerTestGeRatio() {

I think we are getting to the cleanup stage.

Maybe I spoke too soon.

What is the right way to test the new MemoryManager class? It relies on the function memory_usage(), not to mention the event dispatcher. Currently, we wrap memory_usage() in a class method so that we can override it in TestMemoryManager.

I think a better testing strategy would be to add a SystemMemory service class, and inject it into the MemoryManager class. This service class could contain the wrapper for memory_usage(), so we can pass a mocked version of this new service class in the test. our goal should be to get rid of the TestMemoryManager class, injecting whatever mocked objects we need when testing MemoryManager.

The service class might be part of the System module, or it might even be a Core class, since it is not really tied to migration. It might also contain a wrapper for gc_collect_cycles(), so that we can use it in the MemoryLimitExceeded event subscriber as well as in the MemoryManager class. Or it could be passed to the event subscriber through the MigrateMemoryLimitEvent object.

#62
1 Yes, I have changed my workflow locally and some things are still wrong plus I hadn't had time do a self review.
2 No change. There is not standard, my search show many examples similar to this.
3 Oops, it was there and I must hvae reverted it. Fixed now.
4. I am not sure either but guess that is why there is a separate NigrateExecutableMemoryExceededTest.
5 I too prefer prophecy. But there is not yet a policy that I know of to use it. In this instance it would be introducing a second test framework just for one class. I think that is unnecessary and makes the code harder to read.
6 Ok, Make this a followup.
7 I trust PhpStorm less and less these days. It was not my intention to remove the @throws. It has been restored.
8, 9, 10. Fixed
11 - 16 No changes, mostly because of #63.

#63. No opinion right now if that is a good idea or not. However, I would like another opinion on the preferred method for writing the test.

I do not like this idea of improving the test coverage in a follow-up issue.

1. Testing is a core gate.
2. This is a refactoring issue, not a bug fix, so there is no rush.
3. If we need to inject additional dependencies, which seems likely, then that will be a change to the MemoryManager class and its interface.
4. If we decide to pass the SystemMemory object (provisional name) through the dispatched event, then that will be another public change.

Maybe my (3) is bogus because we instantiate MemoryManager as a service. Maybe we will decide not to pass the SystemMemory object with the event, in which case (4) is moot. But let's make that decision now.

We have a solution for testing though, don't we? And constructors are not part of the BC contract. Could we keep this issue tightly scoped and make improvements iteratively?

I guess I am outvoted. If we keep the current testing structure, then we should still address #62.11-16, so back to NW. See the last few lines of #64.

Let's get this moving again.

Some fixes for #62.
11. Fixed. Removed all occurrences of 'Default to NULL' in the doc blocs.
14. Fixed. Can't comment what is usual practice but I see wrappers around protected methods in other migrate tests, such as \Drupal\Tests\migrate\Unit\SqlBaseTest.
15. Removed unhelpful test.
16. Fixed

And removed methods not needed from \Drupal\Tests\migrate\Unit\TestMemoryManager.

That leaves #62, 12 and 13.

I think this is as far as I can go with this. I don't know how to implement changes for #62, 12 and 13

Update patch because #3156879: \Drupal\Component\Utility\Bytes::toInt() - ensure $size is a number type was committed.

Forgot to remove the @todo, ignore previous patch.

The patch in #73 does not apply because #3156879: \Drupal\Component\Utility\Bytes::toInt() - ensure $size is a number type changed Bytes::toInt() with Bytes::toNumber(). I am adding the "Needs reroll" tag and setting the status to NW.

I don't know how to implement changes for #62, 12 and 13

Let me explain #62.13 first, since that is simpler: "Can we add a test case where ensureMemory() returns FALSE?"

In MemoryManagerTest, the data provider returns three ... things. I call them test cases, but maybe that is not the right term. Each of the three has 'expected_ensure_memory' => TRUE,. Can we add one with 'expected_ensure_memory' => FALSE,?

What did I mean by #62.12? The test method in MemoryManagerTest has these lines:

    if ($cleared_memory_usage) {
      $memory_manager->reclaim();
    }
    $result = $memory_manager->ensureMemory();

The ensureMemory() method starts with these lines:

    if (!$this->isLimitExceeded()) {
      return TRUE;
    }
    $this->dispatchEvent(self::PRE_RECLAIMED);

I think the test would be more like "real life" if, instead of calling reclaim() directly from the test method, it called reclaim() indirectly, either through the mocked event dispatcher or from a test module that listens for the event. (For a unit test, I think the second option is not possible.)

Reroll the patch and change references to deprecated Bytes:toInt to Bytes:toNumber.

instead of calling reclaim() directly from the test method, it called reclaim() indirectly, either through the mocked event dispatcher

I tried and was unable to do this, please show me how.

I can't call reclaim from the mocked dispatcher easily, but I can call it from the test managers dispatch event. How's this?

Thanks for improving the test, and I apologize for letting this sit so long.

1. Calling reclaim() this way is pretty close (good enough!) to what I asked for in #74:

    +++ b/core/modules/migrate/tests/src/Unit/MemoryManagerTest.php
    @@ -0,0 +1,265 @@
    ...
    +  public function reclaim() {
    +    $this->memoryUsage = $this->clearedMemoryUsage;
    +    return $this;
    +  }
    ...
    +  protected function dispatchEvent($phase) {
    +    parent::dispatchEvent($phase);
    +    if ($phase === self::PRE_RECLAIMED && $this->clearedMemoryUsage) {
    +      $this->reclaim();
    +    }
    +  }

   The reclaim() method is no longer used outside TestMemoryManager, so it certainly does not need to be public. It is only used here, so let’s inline it and save one level of indirection.

   If you do not like the idea of inlining it, can we test if($this->clearedMemoryUsage) inside reclaim() instead of in dispatchEvent()?

2. It took me a while to realize that this is adding an assertion (+1 for that!) and is independent of the previous point:

    +   * @param bool $will_reclaim
    +   *   Indicates that the manager will attempt to reclaim memory.
    +   * @dataProvider providerTestEnsureMemory
    +   */
    +  public function testEnsureMemory($reclaim_threshold, $memory_threshold, $memory_limit, $memory_usage, $cleared_memory_usage, $expected_ensure_memory, $will_reclaim) {
    +    if ($will_reclaim) {
    +      $this->eventDispatcher
    +        ->expects($this->exactly(2))
    +        ->method('dispatch');
    +    }
    ...
    +  public function providerTestEnsureMemory() {
    +    return [
    +      // Tests memoryExceeded method when a new batch is needed.
    +      'MemoryExceededNewBatch' => [
    ...
    +        'will_reclaim' => FALSE,
    +      ],
    +      // Tests memoryExceeded method when enough is cleared.
    +      'testMemoryExceededClearedEnough' => [
    ...
    +        'will_reclaim' => TRUE,
    +      ],
    +      // Tests memoryExceeded when memory usage is not exceeded.
    +      'testMemoryNotExceeded' => [
    ...
    +        'will_reclaim' => FALSE,
    +      ],
    +      // Tests memoryExceeded method when not enough is cleared.
    +      'testMemoryExceededNotCleared' => [
    ...
    +        'will_reclaim' => TRUE,
    +      ],
    +    ];
    +  }

   First of all, let’s restore the "blank" line (just *) before the @dataProvider comment.

   We do not need the new variable $will_reclaim. It is just the return value of isLimitExceeded(). I think we should rename the variable to match: either $limit_exceeded or $is_limit_exceeded. Then let’s calculate it in the test function. That will make it easier to add new test cases. Finally, lets assert that dispatch() is never called when this variable is FALSE. Something like this:

    $limit_exceeded = $memory_threshold && $memory_usage / $memory_limit > $memory_threshold;
    $this->eventDispatcher
      ->expects($this->exactly($limit_exceeded ? 2 : 0))
      ->method('dispatch');

    +++ b/core/modules/migrate/src/MemoryManager.php
    @@ -0,0 +1,148 @@
    +  public function ensureMemory() {
    +    if (!$this->isLimitExceeded()) {
    +      return TRUE;
    +    }
    +    $this->dispatchEvent(self::PRE_RECLAIMED);
    +    // Re-check the reclaim threshold to ensure we reclaimed enough to
    +    // continue.
    +    if ($this->isLimitExceeded($this->memoryReclaimThreshold)) {
    +      $this->dispatchEvent(self::STILL_EXCEEDED);
    +      return FALSE;
    +    }
    +    $this->dispatchEvent(self::REDUCED_ENOUGH_TO_CONTINUE);
    +    return TRUE;
    +  }

Looking at this again, I think it would be simpler to combine the second call to dispatchEvent() and the return statement:

    $limit_exceeded = $this->isLimitExceeded($this->memoryReclaimThreshold);
    $this->dispatchEvent($limit_exceeded ? self::STILL_EXCEEDED : self::REDUCED_ENOUGH_TO_CONTINUE);
    return !$limit_exceeded;

Another option is to rewrite MigrateMemoryLimitEvent so that it has a MemoryManager property (and getter) instead of three numeric properties (and getters). Then event subscribers would have to replace $event->getUsageInBytes() with $event->getMemoryManager()->getUsageInBytes().

Then, in the TestMemoryManager class, keep reclaim() as a public method (ignoring my suggestion in #79.1). Do not override dispatchEvent().

Finally, in the mock event dispatcher, do not just assert how many times dispatch() will be called. Have the call to dispatch() check $event->getPhase() and conditionally call $event->getMemoryManager()->reclaim().

That seems like a lot of work just to make the test more like "real life". I have not decided yet whether this is a case of making the "real" code more complex just so that it can be tested or whether this is a case of better code structure suggested by the testability mandate. What do you think?

Finally, in the mock event dispatcher, ... [h]ave the call to dispatch() check $event->getPhase() and conditionally call $event->getMemoryManager()->reclaim().

That is part of my suggestion in #81. Usually, when we mock a method, all we do is specify

• how many times it is called
• what arguments it receives
• what it returns

Is it possible to have the mock method do something? I am not sure, but I think it is possible using returnCallback(). The callback receives the arguments(s) of the mock method, and the callback can be an anonymous function, so it can do ... anything you want.

• Example 9.7: Stubbing a method call to return a value from a callback in the PHPUnit docs
• Modifying objects in returnCallback() of PHPUnit Mocks on StackOverflow (see the code snippet in the question)

In #81, I wrote,

I have not decided yet whether this is a case of making the "real" code more complex just so that it can be tested or whether this is a case of better code structure suggested by the testability mandate.

After thinking about it a bit, I like this idea. Let's pass the memory manager itself, not the various numbers, in the event object.

If I look too long at TestMemoryManager, I start to think that getUsageInBytes() is just a static getter. In fact, it is a wrapper for the very non-static memory_get_usage(). Currently, we call it once, save the result in an event object, then dispatch that event. The one event listener in this patch does not even check the value! If there are any other event listeners, they will get the memory usage saved in the event when it was created.

I think it would be better to pass the MemoryManager object in the event. Then each event listener can either ignore the memory usage or ask the memory manager for the current value.

If you make this change, then I think we can postpone the rest of my suggestion in #81 and #82 (the part dealing with using returnCallback()) to a follow-up issue. For this issue, I would still like to see #79.2 and #80 implemented.

Since we want the methods getUsageInBytes(), getLimit(), getUsageRatio() to be available to the event listeners, make them public and add them to MemoryManagerInterface.

I searched for returnCallback() and found several examples in core tests. For example, MakeUniqueEntityFieldTest in the Migrate module (thanks to @chx in #2147815: Migrate: add an entity deduplication process plugin) and FormErrorHandlerTest in the Inline Form Errors module.

We discussed this issue at #3175894: [meeting] Migrate Meeting 2020-10-15. Based on the point I made in #83, @heddn changed his mind, so I am no longer outvoted. @heddn also pointed out #3000050: Replace \Drupal\Component\Utility\Environment::checkMemoryLimit with a PHP memory checking helper class and service, which is already linked as a related issue. I am marking this issue as postponed on that one.

Follow-up to #83: the problem with passing the MemoryManager object in the dispatched event is that we do not want subscribers to have access to the ensureMemory() method. So we really should have the getUsageInBytes() and similar methods in a separate class. That is pretty much what I suggested in #63 and #67. It is also pretty much what @mondrake suggested in #31 a few months earlier.

#3000050: Replace \Drupal\Component\Utility\Environment::checkMemoryLimit with a PHP memory checking helper class and service was closed as "won't fix". @alexpott says that we should be testing low-memory scenarios with ini_set('memory_limit', ...) and by actually consuming memory:

I think a test can influence the remaining memory by consuming said memory. The amount of memory consumed by a process is very predictable. It's not difficult to programatically create an image that PHP will load into memory that will use a predictable amount of memory. So for example, with the color module it will be possible to set a memory limit that triggers the error.

and

Also mocking this gives a fake sense of testing in a low memory environment - because it's not actually a low memory environment. For that we need to work on things like #2495411: Make simpletest fail a test when it detects pages that need more than 64MB

This is blocking, #2688297: File migration slows down and eats more and more memory, eventually stops

+++ b/core/modules/migrate/src/MemoryManager.php
@@ -0,0 +1,148 @@
+    $this->memoryLimit = ($memory_limit == -1)
+      ? PHP_INT_MAX
+      : Bytes::toNumber($memory_limit);

I'm working on a very large migration of users on a site. The memory regularly goes up to 3 or 4GB before finally dying under its own weight. Instead of letting it grow to infinite size, I added an artificial limit of 1.25GB and let this cache clear logic execute. That has helped immensely.

We should let this memory limit be an optional injected parameter. I could see other's wanting the same control over things.

That sounds like a good idea!

#89 is a good idea. I think that should be done in a separate issue.

Just rerolling the patch.

It isn't clear to me what the next step is. #86 is too vague for me.

Re-roll for 10.1.x, as I'm interested in testing this out for a migration which is having memory issues.

Re-roll of 91 to account for CR-3159012: Symfony Event class deprecated, EventDispatcher::dispatch() argument order changed.

And one more for coding standards.

97 was an empty file, not a patch. Some days ... apologies all.

On next update I think we can also remove the "postponed" comment in ID.

All new properties need types and should use constructor promotion

1. +++ b/core/modules/migrate/src/MemoryManager.php
   @@ -0,0 +1,148 @@
   +   * @var float
   ...
   +  protected $memoryReclaimThreshold;
   ...
   +   * @var float
   ...
   +  protected $memoryThreshold;
   ...
   +   * @var int
   ...
   +  protected $memoryLimit;
   ...
   +   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
   ...
   +  protected $dispatcher;
   

   needs to be typed and could be part of constructor

2. +++ b/core/modules/migrate/src/MemoryManager.php
   @@ -0,0 +1,148 @@
   +   * @param int|string $memory_limit
   ...
   +  public function __construct(EventDispatcherInterface $dispatcher, $reclaim_threshold, $memory_threshold, $memory_limit = NULL) {
   

   int|string|null

Added type-hints to fix #99

It's often less arguments passed, so no reason in deprecation and event dispatcher also could be created in constructor to minimize amount of useless extra function calls (will file follow-up)

Here's a fix for CS and fix test - memory_usage always int, never null

To run tests the method is required, also fix one more place (test still fails)

Fix last broken test

Final polishing

Re-rolled last patch as MR

There are some bullets in the remaining tasks that can't tell if they still apply. Could they be marked out if no longer applicable?

tests pass

I've gone through and responded to most of the feedback mentioned in the IS. The requests in #82 & #83 are not super clear what is being asked given that the EnvironmentMemory object didn't materialize. Can we cross them off as well?

The LRU cache finally landed in Drupal core (see related issues). It is likely that migrate can remove its memory handling as a result.

Yes we might be able to delete this code altogether after #3498154: Use LRU Cache for static entity cache.

Appears to need a rebase.

Wonder if the follow up tag is still needed?

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

As the issue #3498154: Use LRU Cache for static entity cache is postponed it makes sense to file follow-up to clean-up and add TODO there, for #113

and needs rebase/reroll too

Re-rolled.

I'm unsure what more needs to be done based on #113 and #115

Left a review on the MR, I don't think this is really going to do anything any more now that the LRU cache issue is in, and we should consider removing this code instead.

I'm gonna have a go at repurposing this issue; starting by editing the IS then making the changes

Removed Memory Manager and event subscriber

Looks like the phpstan baseline might need to be regenerated: https://git.drupalcode.org/issue/drupal-3006750/-/jobs/5474912

How to do this: https://www.drupal.org/node/3426891

Updated the title to match the new direction of this issue to remove memory management code in migrate. Also removed the "Needs follow up" tag, since it referred to messaging in migrate that won't be relevant anymore.

Rebased and updated phpstan baseline.

I was about to update the CR https://www.drupal.org/node/3139212 to reflect that memory management has been removed from MigrateExecutable, but I just read CR https://www.drupal.org/node/3512407 associated with #3498154: Use LRU Cache for static entity cache:

Sites running migrations on some (free) hosting plans with a memory limit of, say, 128MB may now get the following error.

"Memory usage is now @usage (@pct% of limit @limit), not enough reclaimed, starting new batch"

This is because the memory is no longer manually freed up whenever we do a memory usage check.

There's a chance then on hosting plans with low memory that migrations will OOM and fail now. I wonder if something should remain in MigrateExecutable to inform that memory is getting low and to reduce entity.memory_cache.slots.

One idea to address #125 is introducing a memory limit/memory % threshold to the LRU cache instead.

I reviewed the changes on the MR, and I looked at what was left after all the deletions. I made one little suggestion on the MR: back to NW for that. Other than that, I like what I see.

I thought about how to test this issue. I looked at the related issue #2688297: File migration slows down and eats more and more memory, eventually stops. Reading through all the comments, I see that several people reported this problem, but no one was able to provide steps to reproduce it. I decided to Postpone that issue (maintainer needs more information) with a note not to close the issue.

Then I remembered my own experience on my first D7-D8 migration. We had about 100K users on the D7 site, and the d7_user migration started out fast but then slowed to a crawl. (Eventually, I discovered the batch_size option for any source plugin extending SqlBase and that was enough for that project.)

I created a new D7 site and used the devel module (drush generate-users) to create 100K users.

Then I configured memory_limit to 96M for my Drupal 10/11 site. I confirmed with drush eval "echo ini_get('memory_limit') . PHP_EOL".

I decided to test on 10.6.x, which does not include the commit for #1199866: Add an in-memory LRU cache, on 11.x, and on the branch from MR 5156.

Testing steps

1. Check out the branch.
2. Install drush and PHP dependencies: composer require drush/drush.
3. Install Drupal (with the Standard profile): drush si.
4. Enable the migrate_drupal module: drush en migrate_drupal.
5. Import users: drush mim d7_user_role,d7_user.
6. Roll back the migration: drush mr d7_user.
7. Re-import users: drush mim d7_user.

Something has improved since my first D7-D8 migration: on all three branches, the initial migration (Step 5) finished cleanly, without the slow-down that I remembered. That is why I added Steps 6 and 7.

Test results

Testing on 10.6.x

$ drush -v mr d7_user && drush -v mim d7_user
 [info] Drush bootstrap phase: bootstrapDrupalRoot()
...
 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% < 1 sec
 [notice] Processed 3 items (3 created, 0 updated, 0 failed, 0 ignored) in 0.3 seconds (644.7/min) - done with 'd7_user_role'
  92791/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░]  92% 22 mins, 2 secs [warning] Memory usage is 81.64 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 76.74 MB (80% of limit 96 MB), not enough reclaimed, starting new batch
 100001/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 22 mins, 2 secs
 [notice] Processed 92791 items (92791 created, 0 updated, 0 failed, 0 ignored) in 1321.6 seconds (4212.5/min) - done with 'd7_user'

$ drush -v mr d7_user && drush -v mim d7_user
 [info] Drush bootstrap phase: bootstrapDrupalRoot()
...
   6146/100001 [▓░░░░░░░░░░░░░░░░░░░░░░░░░░░]   6% 8 mins, 54 secs [warning] Memory usage is 81.66 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 16.43 MB (17% of limit 96 MB), reclaimed enough, continuing
  12193/100001 [▓▓▓░░░░░░░░░░░░░░░░░░░░░░░░░]  12% 17 mins, 16 secs [warning] Memory usage is 81.62 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 16.98 MB (18% of limit 96 MB), reclaimed enough, continuing
  18174/100001 [▓▓▓▓▓░░░░░░░░░░░░░░░░░░░░░░░]  18% 25 mins [warning] Memory usage is 81.61 MB (85% of limit 96 MB), reclaiming memory.
...
  86034/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░░]  86% 1 hr, 17 mins [warning] Memory usage is 81.6 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 24.17 MB (25% of limit 96 MB), reclaimed enough, continuing
  91382/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░]  91% 1 hr, 18 mins [warning] Memory usage is 81.66 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 24.37 MB (25% of limit 96 MB), reclaimed enough, continuing
 100001/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 1 hr, 18 mins
 [notice] Rolled back 92791 items - done with 'd7_user'
 [info] Drush bootstrap phase: bootstrapDrupalRoot()
...
   3099/100001 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░]   3% 26 secs [warning] Memory usage is 81.66 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 26.67 MB (28% of limit 96 MB), reclaimed enough, continuing
   6083/100001 [▓░░░░░░░░░░░░░░░░░░░░░░░░░░░]   6% 52 secs [warning] Memory usage is 81.62 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 27.89 MB (29% of limit 96 MB), reclaimed enough, continuing
   9049/100001 [▓▓░░░░░░░░░░░░░░░░░░░░░░░░░░]   9% 1 min, 16 secs [warning] Memory usage is 81.64 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 29.17 MB (30% of limit 96 MB), reclaimed enough, continuing
...
  63550/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░░░░░░░░░]  63% 9 mins, 5 secs [warning] Memory usage is 81.66 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 51.12 MB (53% of limit 96 MB), reclaimed enough, continuing
  65148/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░░░░░░░░]  65% 9 mins, 18 secs [warning] Memory usage is 81.65 MB (85% of limit 96 MB), reclaiming memory.
 [warning] Memory usage is now 51.75 MB (54% of limit 96 MB), reclaimed enough, continuing
  65530/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░░░░░░░░]  65% 9 mins, 22 secsPHP Fatal error:  Allowed memory size of 100663296 bytes exhausted (tried to allocate 2097160 bytes) in /var/www/html/vendor/drush/drush/src/Drupal/Migrate/MigrateExecutable.php on line 482

Fatal error: Allowed memory size of 100663296 bytes exhausted (tried to allocate 2097160 bytes) in /var/www/html/vendor/drush/drush/src/Drupal/Migrate/MigrateExecutable.php on line 482
 [warning] Drush command terminated abnormally.
Failed to execute command `drush -v mim d7_user`: exit status 1

Conclusion: even with the memory management in MigrateExecutable, this test ended with a PHP fatal error.

Testing on 11.x

$ drush -v mim d7_user_role,d7_user
 [info] Drush bootstrap phase: bootstrapDrupalRoot()
...
 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% < 1 ms
 [notice] Processed 3 items (3 created, 0 updated, 0 failed, 0 ignored) in 0.3 seconds (700.9/min) - done with 'd7_user_role'
 100001/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 26 min, 33 s
 [notice] Processed 100001 items (100001 created, 0 updated, 0 failed, 0 ignored) in 1592.6 seconds (3767.5/min) - done with 'd7_user'

$ drush -v mr d7_user && drush -v mim d7_user
 [info] Drush bootstrap phase: bootstrapDrupalRoot()
...
 100001/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 1 h, 28 min
 [notice] Rolled back 100001 items - done with 'd7_user'
 [info] Drush bootstrap phase: bootstrapDrupalRoot()
...
 100001/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 14 min, 22 s
 [notice] Processed 100001 items (100001 created, 0 updated, 0 failed, 0 ignored) in 862.7 seconds (6955.3/min) - done with 'd7_user'

Conclusion: after #1199866: Add an in-memory LRU cache (and perhaps other changes) the memory management in MigrateExecutable is never triggered.

Testing on 3006750-migrate-memory-management (after merging with 11.x)

$ drush -v mim d7_user_role,d7_user
 [info] Drush bootstrap phase: bootstrapDrupalRoot()
...
 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% < 1 ms
 [notice] Processed 3 items (3 created, 0 updated, 0 failed, 0 ignored) in 0.2 seconds (739.5/min) - done with 'd7_user_role'
 100001/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 25 min, 27 s
 [notice] Processed 100001 items (100001 created, 0 updated, 0 failed, 0 ignored) in 1526.9 seconds (3929.6/min) - done with 'd7_user'

$ drush -v mr d7_user && drush -v mim d7_user
 [info] Drush bootstrap phase: bootstrapDrupalRoot()
...
 100001/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 1 h, 26 min
 [notice] Rolled back 100001 items - done with 'd7_user'
 [info] Drush bootstrap phase: bootstrapDrupalRoot()
...
 100001/100001 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 20 min, 36 s
 [notice] Processed 100001 items (100001 created, 0 updated, 0 failed, 0 ignored) in 1236.4 seconds (4852.8/min) - done with 'd7_user'

Conclusion: Removing the memory management in MigrateExecutable does not make any difference. This is the expected result, given the results of testing on 11.x.

The "blocker" issue tag was added in Comment #87, since this issue blocks #2688297: File migration slows down and eats more and more memory, eventually stops. Since I Postponed that issue (maintainer needs more information), I am removing that tag.

This issue already has the "Needs change record updates" issue tag, but I am also adding an item to the Remaining tasks in the issue summary. The change record needs a complete rewrite, not just an update. (Or maybe this issue no longer needs a change record.)

@benjifisher thank you a lot for testing in #127

applied suggested wording and only CR and summary needs check

@andypost:

Thanks for updating the MR.

As I said in #127, that update was the only change I wanted to see, so the issue is now RTBC.

Since @benjifisher successfully tested with memory @ 96M in #127, seems like OOM concerns in #125 could be an edge case, if anything. (Though technically recommended Drupal PHP memory requirement are as low as 64MB: https://www.drupal.org/docs/getting-started/system-requirements/php-requ...).

The CR https://www.drupal.org/node/3139212 does need an update, though, but otherwise RTBC +1.

Updated the CR at https://www.drupal.org/node/3139212

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!