Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix performance issue #2774

Merged
merged 1 commit into from Aug 18, 2022
Merged

Conversation

ossinkine
Copy link
Contributor

I faced with performance issue on my project. When I run rector process on big number of files it takes a lot of time. I profiled this command and found that NodeScopeResolver is decorated on each node processing. As result there is a huge staketrace:
Screenshot_1

I moved decoration to constructor to do it once and it boost performance a lot.

@ossinkine
Copy link
Contributor Author

Looks like tests failing is not related to my changes

@TomasVotruba
Copy link
Member

This would be a great improvement 👍

What are the before/after numbers on how big project?

@ossinkine
Copy link
Contributor Author

We have about 4k files. Before it takes about 30 minutes and sometime was freezes and after about 6 minutes.

@TomasVotruba
Copy link
Member

TomasVotruba commented Aug 17, 2022

I've retriggered the CI, but it seems to fail because of this change.

The CI is passing well, at least 1 minute ago: #2775

@TomasVotruba
Copy link
Member

We have about 4k files. Before it takes about 30 minutes and sometime was freezes and after about 6 minutes.

Wow, that's amazing 😮

@ossinkine
Copy link
Contributor Author

I rebased my branch onto main and tests have passed

@TomasVotruba
Copy link
Member

Thanks you 👍 Let's get this to main, I'm eager to try it out 😄

@TomasVotruba TomasVotruba merged commit f936077 into rectorphp:main Aug 18, 2022
@TomasVotruba
Copy link
Member

I tried it on Symplify project and both before/after this change take 30 seconds.

Probably depends on used autoload paths. Could you share your rector.php config?

@samsonasik
Copy link
Member

Looking at last 2 downgrade builds for rector-src in main branch for source + vendor, they seems faster (only 3 minutes) which seems means there is speed improvement

@TomasVotruba
Copy link
Member

Could you link those builds? I'd like to check

@samsonasik
Copy link
Member

I probably was miss click, its there:

@TomasVotruba
Copy link
Member

TomasVotruba commented Aug 18, 2022

Interesting... 👏 a week ago it was even 4:20

@ossinkine
Copy link
Contributor Author

Hi @TomasVotruba,

I'm, using the following config:

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

    $rectorConfig->symfonyContainerXml(__DIR__.'/var/cache/test/App_KernelTestDebugContainer.xml');
    $rectorConfig->importNames();
    $rectorConfig->importShortClasses(false);
    $rectorConfig->disableParallel();

    // PHP
    $rectorConfig->import(LevelSetList::UP_TO_PHP_81);
    $rectorConfig->import(SetList::CODE_QUALITY);
    $rectorConfig->import(SetList::DEAD_CODE);
    $rectorConfig->import(SetList::TYPE_DECLARATION_STRICT);

    // Symfony
    $rectorConfig->import(SymfonyLevelSetList::UP_TO_SYMFONY_54);
    $rectorConfig->import(SymfonySetList::SYMFONY_STRICT);

    // Doctrine
    $rectorConfig->import(DoctrineSetList::DOCTRINE_ORM_29);
    $rectorConfig->import(DoctrineSetList::DOCTRINE_CODE_QUALITY);
    $rectorConfig->import(DoctrineSetList::ANNOTATIONS_TO_ATTRIBUTES);

    // PHPUnit
    $rectorConfig->import(PHPUnitLevelSetList::UP_TO_PHPUNIT_90);
    $rectorConfig->import(PHPUnitSetList::PHPUNIT_CODE_QUALITY);
    $rectorConfig->import(PHPUnitSetList::PHPUNIT_EXCEPTION);
    $rectorConfig->import(PHPUnitSetList::PHPUNIT_SPECIFIC_METHOD);
};

@TomasVotruba
Copy link
Member

TomasVotruba commented Aug 25, 2022

Thanks for sharing 👍

It seems quite rich on sets 💪
I think the performance boost will be most likely connected with your exact codebase.

@ossinkine
Copy link
Contributor Author

ossinkine commented Aug 25, 2022

@TomasVotruba Are you going to release this?

@TomasVotruba
Copy link
Member

Yes :)
We usually release once a 2-3 weeks, when we collect more features and bug fixes.

@pheller24
Copy link

Hey, I just wanted to leave my feedback here: really nice improvement, our CI step went from ~10m to under 5min. Even the 🌳 🌲 🌴 love your work (both of you)

@TomasVotruba
Copy link
Member

@pheller24 Thank you for reaching out 👍 ❤️

I love to see huge improvements... all credit goes to @ossinkine 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants