rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.71k stars 686 forks source link

Performance issues since upgrading - 3x slower #8235

Closed jackbentley closed 1 year ago

jackbentley commented 1 year ago

Previously running version 0.15.17 which took about 30-40s without cache. This has jumped to 2m+ after upgrading to any version 0.15.18 or above.

The only change between the below runs is the rector/rector composer package. All code, config and other libraries remain the same between runs.

I've noticed when trying to compare and undo individual changes that replacing the phpstan.phar file in 0.15.18 with the one in 0.15.17 that the time gets cut back down. This would be downgrading PHPStan from 0.10.1 to 0.9.18. I'm unsure if this means it's an issue in PHPStan or an issue in the way Rector utilises PHPStan. This seems to work up until 0.17.0 at which point you start getting internal errors - however time is still quicker at ~30s.

Is there anything I'm missing to get the times back down? configs, etc?

Side note: why is there a dependency on PHPStan if Rector uses it's own version of PHPStan anyway?

$ composer update rector/rector
$ time bin/rector --dry-run --clear-cache

# 0.15.17
real    0m30.782s
user    4m18.359s
sys 0m33.399s

# 0.15.18
real    3m0.307s
user    24m4.009s
sys 3m20.697s

# 0.15.25
real    2m28.463s
user    20m44.439s
sys 1m4.965s

# 0.16.0
real    2m21.204s
user    19m46.855s
sys 0m47.092s

# 0.17.13
# NB: progress bar disappears, lots of output for every rule being run
real    2m40.135s
user    20m17.253s
sys 2m6.614s

# 0.18.4
# NB: progress bar still gone, no output until result - appears like it's hanging but hasn't
real    1m58.722s
user    16m50.567s
sys 0m52.365s
TomasVotruba commented 1 year ago

We do not support the 0.17 and older anymore. Please, upgrade to Rector 0.18. To improve performance, we'd need exact Rector code changes and their affect on exact project code, as general numbers doesn't give us any clue what exactly is the bottleneck.

Side note: why is there a dependency on PHPStan if Rector uses it's own version of PHPStan anyway?

That's actually pretty good question :+1: I think we could drop that.

samsonasik commented 1 year ago

@TomasVotruba iirc, the phpstan dependency is used on purpose as we can't extract phar via box in the past as cause too many error, and to avoid miss/different version requirement

TomasVotruba commented 1 year ago

@samsonasik That is true, but we already require in the composer.json of the compiled rector/rector package - https://github.com/rectorphp/rector/blob/1d1339dd537962a467ce1f7e6fab9b8955c34677/composer.json#L11

So it's installed on any project anyway.

I think it would be safe to drop it during the prefixed Rector build, as we do not change it and use phpstan/phpstan in the project.

samsonasik commented 1 year ago

Rector seems read phpstan from its prefixed vendor and the compiled autoloaded load that, that's why when phpstan have new parameter config, rector need to rebuild, it seems a dependency load config seems need to be resolved if prefixed vendor/phpstan removed.

TomasVotruba commented 1 year ago

@samsonasik Could you point me to the code that is handling this? I'll check it

samsonasik commented 1 year ago

@TomasVotruba it seems loaded early on autoload_files.php, see

https://github.com/rectorphp/rector/blob/1d1339dd537962a467ce1f7e6fab9b8955c34677/vendor/composer/autoload_files.php#L11

that's why prefixed vendor always loaded early.

TomasVotruba commented 1 year ago

@samsonasik I see, I might try few experiments in the build and will report back with results :)

samsonasik commented 1 year ago

Also on other files

  1. vendor/composer/autoload_static.php

https://github.com/rectorphp/rector/blob/1d1339dd537962a467ce1f7e6fab9b8955c34677/vendor/composer/autoload_static.php#L12

  1. vendor/composer/installed.php

https://github.com/rectorphp/rector/blob/1d1339dd537962a467ce1f7e6fab9b8955c34677/vendor/composer/installed.php

jackbentley commented 1 year ago

We do not support the 0.17 and older anymore. Please, upgrade to Rector 0.18. To improve performance, we'd need exact Rector code changes and their affect on exact project code, as general numbers doesn't give us any clue what exactly is the bottleneck.

That's what I'm currently trying to do but have hit these performance issues with the latest version 😂

It's a proprietary code base so I can't share the code but can share config if that would help you at all? There are no rector changes and is all OK on 0.15.17. On 0.16.0 it tries to fix a few things (although I've ran it with --dry-run) and runs in the same amount of time with the patched phpstan.phar file as 0.15.17 does with no changes.

I'll see if I can reproduce the performance issues in a repo or on an open source project.

TomasVotruba commented 1 year ago

Config would not help, as that's like debugging in the dark without knowing the light switch is outside the room :) it could be anything from cache invalidation, bug in PHPStan, bug in php stubs, Symfony or Laravel switch and so on.

Since 0.18 we also run on Laravel container, so most of changes/fixes up-to 0.17 might not be relevant anymore.

It might be useful if you can pinch it down to exact commit between the 2 tags the performance dropped for you: https://github.com/rectorphp/rector/compare/0.15.17...0.15.18

There is 11 commits, so you can try one by one and pinch down the tricky one.

jackbentley commented 1 year ago

Ok, fair enough.

I've gone through that change set already and tried most of them one by one with no effect. As I say, the thing that made the difference is swapping out the PHPStan phar version. I tried going through the changes between PHPStan versions but there's far too many to go through manually with my little knowledge of internals for either Rector or PHPStan.

TomasVotruba commented 1 year ago

I see. In that case it would be somewhere in PHPStan it's dependency tree. That's a rabit hole by now, as it's almost 9 months in past. We can't do anything about this.

TomasVotruba commented 1 year ago

Thanks for the ping about PHPStan duplicity though... it gave me nudge to handle it :)

https://github.com/rectorphp/rector/commit/941ed47703527a25c56841000c16a4c6b5bddc78

jackbentley commented 1 year ago

I've found what the issue was/is. I found it while disabling parallel processing in order to get a cachegrind profile to compare versions with.

It seems like when an error occurs, in the parallel spawned process, it's not reported. They are reported when using --debug with parallel enabled, though. This may have been fixed in a newer version but would be a good idea to check.

When running on 0.15.17 with parallel disabled it spits out a lot of errors along the lines of "Call to undefined method PHPStan\Type\StringType::getObjectClassNames()". On line: 47.

When running with --debug (with or without parallel disabled) it shows these errors are coming from the PHPStan extensions. It seems like Rector's internal PHPStan is picking up extensions installed in the project. These extensions may not be compatible with the version of PHPStan that is shipped with Rector.

Specifically, I had phpstan/phpstan-phpunit @ 1.3.13 which is not compatible with PHPStan 1.9.17 which is what is packed with Rector 0.15.17. I would assume issues with PHPStan extensions would be fixed by your above commit.

So, it turns out the slower time is actually correct. It was only running faster because it was skipping a lot of files (and not telling me).