regular terminating and respawning of processes by Flyingmana · Pull Request #3722 · rectorphp/rector-src

Flyingmana · 2023-05-01T16:54:20Z

Context

As described here rectorphp/rector#7891
I had some issues with memory usage, which caused some issues on a very large project.

I tried out this PR #3563 but as described in #3563 (comment) it was not enough for my usecase.

What is this PR doing

This PR introduces the ability to "restart" spawned workers to reset their memory usage.
In the current Draft it is doing this hardcoded after 10 Iterations. Which should be high enough to not affect the normal usage, but low enough to improve the situation for big projects.
And its abstracted into a property, so adding a setting later is relatively easy. (later, because it opens up questions on how to configure the parallelization properly, especially the conditions for the restarting)

What I changed

I basically just added the lines 186-195 and moved the process spawning into an own callable, so it can get called repeatedly.

I would like to move more into class properties and methods, but thats not so easy and might make the Review harder

Performance comparison

files per restart = jobsize * (onData iterations till restart)

First one comparison extended with the previously linked PR
#3563 (comment)

-	Run A		Run B	Run C
version	last release	last release	PR 3563	this draft	this draft
parallelization	1	3	3	3	6
files per restart	X	X	X	200	200
Time Real	2m 17s	1m 15s	12m 58s	2m 35s	1m 51s
Time User	2m 02s	2m 43s	32m 23s	6m 28s	8m 34s
totalmemory max	~4G	~6G	~0.5G	~3-4G	~5G

But now also one Run for the Full project (19.943 files), not only a subset

-	Run D
version	this draft	this draft	###
parallelization	6	6	3
files per restart	1000	500
Time Real	X	8m 50s	###
Time User	X	45m 12s	###
totalmemory max	~18G	~11G	###

With 1.000 Files per restart, it still exceeded to much my available memory, but with 500 it did fit into memory and I was able to use 100% of my available CPUs, drastically cutting down the needed Real Time.

updated performance test

Setup:
WSL2, Linux, php 8.1
cpu: physical cores 6, logical 12
Ram: 32 GB (25GB allowed for WSL2)

used config:

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->paths([
        __DIR__ . '/src',
        __DIR__ . '/../symfony',
    ]);

    // usual config
    //$rectorConfig->parallel(120*30,12,200);
    // config for yguedidi
    $rectorConfig->parallel(120*30,12,500, true);

    $rectorConfig->rule(\Rector\Php74\Rector\Ternary\ParenthesizeNestedTernaryRector::class);
};

On a current symfony/symfony checkout, 9629 files
last release: rector 1.6.0

-	Run A	Run B	Run C	Run D	Run E	Run F	Run G	Run H
version	last release	last release	last release	last release	pr-3563	pr-3563	pr-3722	pr-3722
special commands			--no-diffs --no-progress-bar
parallelization	16	6	6	12	12	12	12	12
job size	20	20	20	500	500	200	20	10
files per restart	X	X	X	x	x	x	200	100
Time Real	02m 28s	02m 04s	02m 06s	01m 40s	01m 54s	02m 12s	01m 55s	02m 21s
Time User	26m 49s	11m 48s	12m 02s	14m 44s	17m 28s	22m 39s	21m 24s	26m 12s
process memory	2G	2G ~ 4G	2G ~ 4G	0.6G ~ 3G	0.3G ~ 3G	0.2G ~ 2.8G	0.2G ~ 1.8G	0.2G - 0.7G
totalmemory max	~24G	~18G	~18G	~14G	~9G	5GB	5G	4G

and this one is after merging in the changes from the last release

-	Run J
version	pr-3722 updated
special commands
parallelization	12
job size	20
files per restart	200
Time Real	02m 16s
Time User	25m 14s
process memory	0.2G ~ 0.9G
totalmemory max	6G

samsonasik · 2023-05-01T16:56:19Z

/cc @yguedidi

yguedidi · 2023-05-01T17:08:58Z

@Flyingmana looks like we worked on improving parallel runs on big projects in parallel 😅 my PR is ready for review now :)

yguedidi · 2023-05-01T17:35:24Z

@Flyingmana what if you try #3563 with the proposed configuration in #3563 (comment) (or a similar one adapted to your project)?

Flyingmana · 2023-05-07T23:30:25Z

I hope this PR is better suited for a Review and merge by the limited amount of changes compared to #3563.

packages/Parallel/Application/ParallelFileProcessor.php

yguedidi · 2023-05-08T14:15:09Z

packages/Parallel/Application/ParallelFileProcessor.php

-                break;
-            }
+        $workerCommandLineFactory = $this->workerCommandLineFactory;
+        $this->processSpawner = function(


any need to have it as a property?

you can make it a local variable, and pass it by reference in the use list to use it inside

maybe naming it $startProcessCallable would make it clearer?

I actively wanted to avoid having it as a local variable, because there are to many of them which makes reading and changing the code a lot more complex. I would have preferred to make this an actual method in the class, but this would have needed more rewrites

if a private method is too expensive, to me a local variable is a better fit here than a property, as it's a callable used only locally there

packages/Parallel/Application/ParallelFileProcessor.php

yguedidi · 2023-05-08T14:16:12Z

packages/Parallel/Application/ParallelFileProcessor.php

+                    }
+                    if ($this->processRunCounter[$processIdentifier] >= $this->maxProcessIterations) {
+                        $this->processPool->quitProcess($processIdentifier);
+                        if (is_callable($this->processSpawner)){


how this can be false at this stage?

phpstan complained, I dont think it can be false, but the call structure is complex

by following the suggestion in #3722 (comment) it shouldn't complain, as the local variable will be assigned directly, never null

raymondschouten · 2023-06-12T09:43:14Z

This fix is actually the only thing working for my large application, but for some reason my progress bar isn't going up to 100%:

samsonasik · 2023-06-12T11:29:47Z

@raymondschouten progress bar is probably overlapped on large project, even on current dev-main, that 100% is printed later after " [OK] 1592 files have been changed by Rector" message:

so probably "just fine". Do you have progress bar issue on mini project, eg: 100 files?

@Flyingmana that would be awesome if you can catch a possible bug on progress bar not 100% but done.

Flyingmana · 2023-06-26T21:03:54Z

@raymondschouten progress bar is probably overlapped on large project, even on current dev-main, that 100% is printed later after " [OK] 1592 files have been changed by Rector" message:

so probably "just fine". Do you have progress bar issue on mini project, eg: 100 files?

@Flyingmana that would be awesome if you can catch a possible bug on progress bar not 100% but done.

I debugged it a bit, its quite easy to reproduce with $rectorConfig->parallel(120*30,2,2) for 67 files.
Actually even worse with $rectorConfig->parallel(120*30,1,2), but not when I force Option::PARALLEL to be false.
Project I used to test was

        __DIR__ . '/../symfony/src/Symfony/Component/Templating',
        __DIR__ . '/../symfony/src/Symfony/Component/DomCrawler',

Obvious issue is probably, that \Symfony\Component\Console\Helper\ProgressBar::finish is not triggered on the end.

But actually I think there is a place which does count wrong, and as the counting happens in the same place, where we collect the fileDiffs, there might be a bug which actually loses these Diffs.

You say the progress bar issue happens independently of my changes? Then I would like to check it in a separate Issue/PR

Flyingmana · 2023-06-26T22:10:57Z

Any remaining changes which are still required for this?
I would like to keep the class properties instead of local variables as this makes further refactoring to an easier structure with less complexity possible.

samsonasik

Looks good for me, for progress bar, if you have a chance to check, it should still apply to 100% even overlapped after "Rector is done" message so we have same behavior as before, better if that really resolved ( 100% show before "Rector is done" message)

samsonasik

@Flyingmana I tried your branch, it seems when show the diff, it doesn't show 100% progress bar,

so it maybe overlapped. could you check run to see the progress bar to actually show 100% ?

time bin/rector process src tests rules-tests packages packages-tests

samsonasik · 2023-06-29T10:21:42Z

@TomasVotruba the progress bar may need to be fixed to show 100% on many files

samsonasik · 2023-06-29T11:12:10Z

packages/Parallel/Application/ParallelFileProcessor.php

+
+        for ($i = 0; $i < $numberOfProcesses; ++$i) {
+            // nothing else to process, stop now
+            if ($jobs === []) {


is force progress bar to 100% here possible/correct?

I tried apply progress bar to 100% here, but seems not working, it only show 97%:

➜ rector-src git:(df-parallel) time bin/rector process src tests rules-tests packages packages-tests --dry-run --clear-cache 1490/1530 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░] 97% 1 file with changes =================== 1) packages/Parallel/Application/ParallelFileProcessor.php:3

it seems it can be applied on $inDecoder->on() process on jobs empty:

if ($jobs === []) { $this->processPool->quitProcess($processIdentifier); + $this->rectorOutputStyle->progressFinish(); return; }

The issue with it is it show different total when applied, when on progress, it shows 1530:

➜ rector-src git:(df-parallel) ✗ time bin/rector process src tests rules-tests packages packages-tests --dry-run --clear-cache 0/1530 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░]

when done, it shows 1550:

➜ rector-src git:(df-parallel) ✗ time bin/rector process src tests rules-tests packages packages-tests --dry-run --clear-cache 1550/1550 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 2 files with changes ==================== 1) packages/Parallel/Application/ParallelFileProcessor.php:3

I see, it can actually be forced on reporting on ConsoleOutputFormatter:

public function report(ProcessResult $processResult, Configuration $configuration): void { + if ($configuration->shouldShowProgressBar()) { + $this->rectorOutputStyle->progressFinish(); + }

By this, this can show 100%:

➜ rector-src git:(df-parallel) ✗ time bin/rector process src tests rules-tests packages packages-tests --dry-run --clear-cache 1530/1530 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 1 file with changes =================== 1) packages/Parallel/Application/ParallelFileProcessor.php:3

@Flyingmana I create a commit to your branch 88a755b 👍 to handle progress bar 100% issue.

and move $processRunCounter into class property

samsonasik · 2023-07-12T02:35:58Z

All checks have passed 🎉 @TomasVotruba I think it is ready to give a try 👍

TomasVotruba · 2023-07-12T08:37:40Z

Thank you all for your hard work. Let's give this a try :)

I'll tag a new version, so we have clean plate for testing this one 🙏

samsonasik · 2023-07-12T08:48:48Z

@TomasVotruba @Flyingmana it seems caused error on downgrade so we need to revert

This reverts commit ff05d32.

samsonasik · 2023-07-12T08:54:14Z

Reverted at #4489

TomasVotruba · 2023-07-12T08:55:50Z

@samsonasik Got any reproducer we can use here as a test case?

samsonasik · 2023-07-12T09:04:42Z

I will try 👍 , some process probably forced terminated before it actually complete

Flyingmana mentioned this pull request May 7, 2023

Allow to use a dedicated process for each job #3563

Closed

Flyingmana marked this pull request as ready for review May 7, 2023 23:24

Flyingmana requested a review from TomasVotruba as a code owner May 7, 2023 23:24

samsonasik reviewed May 8, 2023

View reviewed changes

packages/Parallel/Application/ParallelFileProcessor.php Outdated Show resolved Hide resolved

yguedidi suggested changes May 8, 2023

View reviewed changes

raymondschouten mentioned this pull request Jun 12, 2023

Getting memory usage under control rectorphp/rector#7980

Closed

Flyingmana requested a review from samsonasik June 26, 2023 22:11

samsonasik approved these changes Jun 27, 2023

View reviewed changes

samsonasik requested changes Jun 27, 2023

View reviewed changes

TomasVotruba requested a review from samsonasik June 29, 2023 10:08

samsonasik reviewed Jun 29, 2023

View reviewed changes

samsonasik mentioned this pull request Jul 11, 2023

Timeout on LevelSetList::UP_TO_PHP_81 rectorphp/rector#8046

Closed

Flyingmana and others added 9 commits July 12, 2023 07:37

regular terminating and respawning of processes

1d264be

change processPool back to class property

58e5005

move processSpawner to class property

cca3732

remove debug snippets

6a27230

move processIterations limiter to class property

9c3476a

resolve phpstan complaints

2573b17

and move $processRunCounter into class property

resolve phpstan complaint differently

2b17129

switch class property to constant

6be753f

remove local var

71de475

force finish on reporting

88a755b

samsonasik force-pushed the df-parallel branch from 03ca5c7 to 88a755b Compare July 12, 2023 02:33

samsonasik approved these changes Jul 12, 2023

View reviewed changes

TomasVotruba merged commit ff05d32 into rectorphp:main Jul 12, 2023

samsonasik added a commit that referenced this pull request Jul 12, 2023

Revert "regular terminating and respawning of processes (#3722)"

f940800

This reverts commit ff05d32.

samsonasik mentioned this pull request Jul 12, 2023

Revert "regular terminating and respawning of processes" #4489

Merged

samsonasik added a commit that referenced this pull request Jul 12, 2023

Revert "regular terminating and respawning of processes (#3722)" (#4489)

84afeaf

This reverts commit ff05d32.

samsonasik mentioned this pull request Sep 10, 2023

re-spawn worker processes rectorphp/rector#8192

Closed

Uh oh!

Conversation

Flyingmana commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

What is this PR doing

What I changed

Performance comparison

updated performance test

Uh oh!

samsonasik commented May 1, 2023

Uh oh!

yguedidi commented May 1, 2023

Uh oh!

yguedidi commented May 1, 2023

Uh oh!

Flyingmana commented May 7, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondschouten commented Jun 12, 2023

Uh oh!

samsonasik commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flyingmana commented Jun 26, 2023

Uh oh!

Flyingmana commented Jun 26, 2023

Uh oh!

samsonasik left a comment

Choose a reason for hiding this comment

Uh oh!

samsonasik left a comment

Choose a reason for hiding this comment

Uh oh!

samsonasik commented Jun 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samsonasik commented Jul 12, 2023

Uh oh!

TomasVotruba commented Jul 12, 2023

Uh oh!

samsonasik commented Jul 12, 2023

Uh oh!

samsonasik commented Jul 12, 2023

Uh oh!

TomasVotruba commented Jul 12, 2023

Uh oh!

samsonasik commented Jul 12, 2023

Uh oh!

Reviewers

Flyingmana commented May 1, 2023 •

edited

Loading

samsonasik commented Jun 12, 2023 •

edited

Loading