Performance regression since 1.0.5

[mfn]

Bug Report

Subject	Details
Rector version	1.0.5

Runtime of rector did skyrocket with 1.0.5, 1.0.4 was fine.

Before:

 $ time composer rector-tests
> @php vendor/bin/rector process --config=rector-tests.php
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                        
 [OK] Rector is done!                                                                                                   
                                                                                                                        


real    0m3.728s
user    0m3.077s
sys     0m0.478s

After:

 $ time composer rector-tests
> @php vendor/bin/rector process --config=rector-tests.php
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                        
 [OK] Rector is done!                                                                                                   
                                                                                                                        


real    0m39.907s
user    0m38.353s
sys     0m0.881s

In both cases I took a single file out of ~3k and just added a blank line.

In both cases it detected correctly only a single modified file, but the execution time is very much different.

Minimal PHP Code Causing Issue

Config(s)
rector-common.php

<?php declare(strict_types = 1);

use Rector\Config\RectorConfig;
use Rector\Php80\Rector\Catch_\RemoveUnusedVariableInCatchRector;
use Rector\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector;
use Rector\Php80\Rector\ClassMethod\AddParamBasedOnParentClassMethodRector;
use Rector\Php80\Rector\FuncCall\ClassOnObjectRector;
use Rector\Php80\Rector\Identical\StrEndsWithRector;
use Rector\Php80\Rector\Identical\StrStartsWithRector;
use Rector\Php80\Rector\NotIdentical\StrContainsRector;
use Rector\Php81\Rector\Array_\FirstClassCallableRector;
use Rector\Php81\Rector\ClassMethod\NewInInitializerRector;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->rule(AddParamBasedOnParentClassMethodRector::class);
    $rectorConfig->rule(ClassOnObjectRector::class);
    $rectorConfig->rule(ClassPropertyAssignToConstructorPromotionRector::class);
    $rectorConfig->rule(FirstClassCallableRector::class);
    $rectorConfig->rule(NewInInitializerRector::class);
    $rectorConfig->rule(RemoveUnusedVariableInCatchRector::class);
    $rectorConfig->rule(StrContainsRector::class);
    $rectorConfig->rule(StrEndsWithRector::class);
    $rectorConfig->rule(StrStartsWithRector::class);
};

rector-tests.php

<?php declare(strict_types = 1);

use Rector\Config\RectorConfig;
use Rector\PHPUnit\Set\PHPUnitSetList;

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

    $common = require __DIR__ . '/rector-common.php';
    $common($rectorConfig);

    $rectorConfig->sets([
        PHPUnitSetList::ANNOTATIONS_TO_ATTRIBUTES,
    ]);

    $rectorConfig->importNames();
};

Expected Behaviour

Be as fast as before 😅

---

I can't share more details, this is a private repo. But let me know if you've any idea what I could test.

[samsonasik]

we are aware of that :), that's due to the needs of child class detection :

• [Experiment] Rework child classes detection on DynamicSourceLocatorProvider rector-src#5735

I was trying to improve it by cache the classname collections:

• [Performance] Cache classname collection on DynamicSourceLocatorProvider rector-src#5845

but temporary closed due to possible side effect, I will check if there is better way to "cache" without relying on Cache object.

[mfn]

Thanks!

But: why close, when it's not fixed?

The regression is very noticeable on big projects. We might need to roll back to 1.0.4

[samsonasik]

That may require some architecture change, as on parallel, the ->provide() called based on total worker, as individual service, which paths got empty on the first place, which should be called once, if possible.

Closing it as may cause multiple conflict, and better to force change when it is ready.

It will done when it is done :)

[samsonasik]

I created PR:

• [Performance] Avoid $reflector->reflectAllClasses() in every worker on DynamicSourceLocatorProvider rector-src#5877

for it.

[samsonasik]

Closing for now to avoid distractions and conflict, and better peace in my mind.

I am aware of it, and will revisit later with hopefully better solution.

[samsonasik]

I created new PR:

• [Performance] Ensure call $reflector->reflectAllClasses() once on Worker take 3 rector-src#5878

for it.

[mfn]

Based on the mention in rectorphp/rector-src#5879 (comment) , I tested with require --dev rector/rector:dev-main (it installed dev-main 3c1e41f)

dev-main 3c1e41f cold, first run:

 $ time composer rector-tests
> @php vendor/bin/rector process --config=rector-tests.php
 2980/2980 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


 [OK] Rector is done!



real	1m30.967s
user	8m39.800s
sys	0m16.978s

dev-main 3c1e41f: modified a random test file (added empty line):

 $ time composer rector-tests
> @php vendor/bin/rector process --config=rector-tests.php
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


 [OK] Rector is done!



real	0m20.660s
user	0m19.555s
sys	0m0.759s

A bit better, but still too slow:

But still a far cry from 1.0.4 (re-did the same test again)

1.0.4, cold cache (no change):

 $ time composer rector-tests
> @php vendor/bin/rector process --config=rector-tests.php
 2980/2980 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


 [OK] Rector is done!



real	1m13.126s
user	8m20.583s
sys	0m17.299s

1.0.4: modified a random test file (added empty line):

 $ time composer rector-tests
> @php vendor/bin/rector process --config=rector-tests.php
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


 [OK] Rector is done!



real	0m3.500s
user	0m2.855s
sys	0m0.451s

[samsonasik]

Thank you for testing. When there is file change, the class collection need to be re-collected for multiple runs to keep the collection steady.

I was trying to collect classes only when service to get child classes called, but got possible multiple init on:

• [Performance] Move collect class names collection cache to FamilyRelationsAnalyzer when service is used rector-src#5882

I will look more on improvement when possible.

[staabm]

Can someone share a reproducer for this perf problem?

[TomasVotruba]

Reopening to address this clearly.

[mfn]

Thank you very much. I refrained from commenting the previous closures, but it was not how I would have expected to issues to be treated, which were clearly not fixed.

Thus: thank you for acknowledging this ❤️

[samsonasik]

@mfn I created PR to drop children class detection

• [Bc Break] [Alternative] Drop children class detection rector-src#5995

I will let @TomasVotruba to decide ;)

[TomasVotruba]

Thanks @samsonasik for quick work 👍

Thanks for patience to all 🙏 I'm now travelling and connection is sometime challange to get here. I know I should have check this more deeply sooner.

Resolved in rectorphp/rector-src#5995