Implement a max jobs per worker budget by staabm · Pull Request #4965 · rectorphp/rector-src

staabm · 2023-09-10T07:21:51Z

packages/Parallel/Application/ParallelFileProcessor.php

staabm · 2023-09-10T07:24:24Z

will be AFK for a few hours

packages/Parallel/Application/ParallelFileProcessor.php

staabm · 2023-09-10T11:56:22Z

packages/Config/RectorConfig.php

    }

-    public function parallel(int $seconds = 120, int $maxNumberOfProcess = 16, int $jobSize = 20): void
+    public function parallel(int $seconds = 120, int $maxNumberOfProcess = 16, int $jobSize = 15): void


reduced the jobsize, which leads to less memory used

config/config.php need to be updated as well:

rector-src/config/config.php

Line 16 in 038f809

$rectorConfig->parallel(120, 16, 20);

could you also update the different between $jobSize vs MAX_CHUNKS_PER_WORKER ? Thank you.

I dropped the args in this file, as these just reflect the defaults and therefore don't need to be kept in sync

could you also update the different between $jobSize vs MAX_CHUNKS_PER_WORKER ?

where?

here seems ok

↓

#4965 (review)

Looks good 👍

staabm · 2023-09-10T12:04:33Z

packages/Parallel/Application/ParallelFileProcessor.php

                    }

-                    $job = array_pop($jobs);
+                    $jobsChunk = array_pop($jobs);


renaming because the previous name suggested it would be a single job

staabm · 2023-09-10T12:10:39Z

with the parameter adjustments at this point we can run rector across mautic with

    $rectorConfig->paths([
        __DIR__.'/app/',
        __DIR__.'/plugins',
    ]);

with a peak of 14.75 GB. it never grows higher then that:

time php ../rector-src/bin/rector.php --dry-run --no-progress-bar --no-diffs --clear-cache
                                               
 [OK] 10 files would have changed (dry-run) by Rector                                                                   
                                                                                                                       
php ../rector-src/bin/rector.php --dry-run --no-progress-bar --no-diffs   379.85s user 11.00s system 798% cpu 48.939 total

with rector 0.18.2 it peaks at 16.5 GB RAM and takes

➜  mautic git:(5.x) ✗ time php ../rector-src/bin/rector.php --dry-run --no-progress-bar --no-diffs --clear-cache
                                                                                                                        
 [OK] 30 files would have changed (dry-run) by Rector                                                                                                                                                                                      

php ../rector-src/bin/rector.php --dry-run --no-progress-bar --no-diffs   516.54s user 8.36s system 766% cpu 1:08.51 total

=> with this PR (and all the already merged optimizations) we are 20 seconds faster and take ~1.75 GB less memory

samsonasik · 2023-09-10T12:17:40Z

packages/Parallel/Application/ParallelFileProcessor.php

+     *
+     * @var int
+     */
+    private const MAX_CHUNKS_PER_WORKER = 8;


it is a bit ambiguous the different between $jobSize vs MAX_CHUNKS_PER_WORKER, could you write a comment here? Thank you.

samsonasik

@TomasVotruba let's quick merge and revert in case of it doesn't work :)

Thank you @staabm

samsonasik · 2023-09-10T12:41:05Z

@staabm I am testing in CodeIgniter 4 project, when we define:

$rectorConfig->parallel(240, 8, 1);

the procss got 2 times slower, but when we define default value:

$rectorConfig->parallel();

the performance seems equals, I mention you in PR test at:

[Rector] Testing Rector dev-main codeigniter4/CodeIgniter4#7918

so in case other CodeIgniter4 project maintainer has different use case, they can give you feedback faster :)

staabm · 2023-09-10T12:45:49Z

the procss got 2 times slower,

thats expected. the smaller the jobsize is and the smaller the chunks-per-worker is, the slower the rector job will get.
but it also uses less memory.

the higher the jobsize and the higher the chunks-per-worker are, rector gets faster but requires more memory.

its a balance between memory and speed. before this PR all default settings were set for maximum speed but without limits for memory. thats the actual problem the bigger the project gets we analyze.

with this PR we have a mean to cut the memory peaks.

performance and memory also depends whether rector just scans the codebase and does not need todo anything or whether it needs to refactor something

staabm · 2023-09-10T12:51:58Z

@Flyingmana @yguedidi you might be interessted in testing rector@dev-main which now uses process re-spawning to recover from memory leaks and memory spikes

staabm added 2 commits September 10, 2023 09:21

Implement a max jobs per worker budget

c6d7457

Update ParallelFileProcessor.php

500a9a4

staabm commented Sep 10, 2023

View reviewed changes

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

staabm marked this pull request as ready for review September 10, 2023 07:24

staabm requested a review from TomasVotruba as a code owner September 10, 2023 07:24

staabm commented Sep 10, 2023

View reviewed changes

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

staabm added 3 commits September 10, 2023 09:43

fix

e76edbc

docs

377b254

reduce default job size

8ad9388

staabm commented Sep 10, 2023

View reviewed changes

better naming

be59729

staabm commented Sep 10, 2023

View reviewed changes

Update ParallelFileProcessor.php

999bb9b

remove defaults from rector config

6ecaaab

samsonasik reviewed Sep 10, 2023

View reviewed changes

Update ParallelFileProcessor.php

14724bf

samsonasik approved these changes Sep 10, 2023

View reviewed changes

samsonasik merged commit 0717306 into rectorphp:main Sep 10, 2023

samsonasik mentioned this pull request Sep 10, 2023

re-spawn worker processes rectorphp/rector#8192

Closed

staabm deleted the para branch September 10, 2023 12:36

staabm mentioned this pull request Sep 12, 2023

[ci] make use of composer install cache to make parallel jobs faster mautic/mautic#12695

Merged

samsonasik mentioned this pull request Jan 9, 2024

[config] Extract RectorConfigValidator and handle config validation there #5404

Merged

Uh oh!

Conversation

staabm commented Sep 10, 2023

Uh oh!

Uh oh!

staabm commented Sep 10, 2023

Uh oh!

Uh oh!

staabm Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

samsonasik Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

staabm Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

samsonasik Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

TomasVotruba Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

staabm Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

staabm commented Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samsonasik Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

samsonasik left a comment

Choose a reason for hiding this comment

Uh oh!

samsonasik commented Sep 10, 2023

Uh oh!

staabm commented Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm commented Sep 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

staabm commented Sep 10, 2023 •

edited

Loading

staabm commented Sep 10, 2023 •

edited

Loading