Skip to content
/ rector-src Public

[Performance] Use filtered filePaths from ApplicationFileProcessor::configurePHPStanNodeScopeResolver on WorkerRunner::run() #4513

Merged
samsonasik merged 5 commits intomainfrom
pass-by-ref-
Jul 14, 2023
Merged

[Performance] Use filtered filePaths from ApplicationFileProcessor::configurePHPStanNodeScopeResolver on WorkerRunner::run() #4513
samsonasik merged 5 commits intomainfrom
pass-by-ref-

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik samsonasik commented Jul 14, 2023 •
edited
Loading

@TomasVotruba @staabm this is to ensure no need to read unsupported file extensions:

Before

vendor/bin/rector process app/admin/ --clear-cache --debug --clear-cache | grep ".xml" 
  string(55) "/home/abdul/www/kunzmann/app/admin/config/adminmenu.xml"
  string(50) "/home/abdul/www/kunzmann/app/admin/config/i18n.xml"

After

vendor/bin/rector process app/admin/ --clear-cache --debug --clear-cache | grep ".xml"

Fixes rectorphp/rector#8056

@samsonasik samsonasik requested a review from TomasVotruba as a code owner July 14, 2023 18:13
staabm
staabm reviewed Jul 14, 2023
View reviewed changes
src/Application/ApplicationFileProcessor.php Outdated
* @param string[] $filePaths
*/
public function configurePHPStanNodeScopeResolver(array $filePaths, Configuration $configuration): void
public function configurePHPStanNodeScopeResolver(array &$filePaths, Configuration $configuration): void
Copy link
Copy Markdown
Contributor

@staabm staabm Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like after this method was called the "correct list" of files should be contained in $this->nodeScopeResolver->setAnalysedFiles().

could the caller of configurePHPStanNodeScopeResolver get it from there?

Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to return file paths instead 9b38ba5

@samsonasik samsonasik changed the title [Performance] Pass by ref filePaths on ApplicationFileProcessor::configurePHPStanNodeScopeResolver() [Performance] Use filtered filePaths from ApplicationFileProcessor::configurePHPStanNodeScopeResolver on WorkerRunner::run() Jul 14, 2023
@staabm
Copy link
Copy Markdown
Contributor

staabm commented Jul 14, 2023 •
edited
Loading

thanks for investigating. the fix makes sense. I was not able to verify the fix yet.
I like the non-reference passing variant a lot more.

is this something which can be unit tested?

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jul 14, 2023 •
edited
Loading

There are 2 process:

  • non-parallel, I optimize it by filter paths early e325e49
  • parallel, it executed by WorkerRunner

I am not sure if unit test is possible, probably e2e is needed and possibly need in separate PR if needed, but seems the change should already clear :).

I will merge it as is and let you test dev-main fo it

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jul 14, 2023

All checks have passed 🎉 @TomasVotruba @staabm let's merge and test 👍

@samsonasik samsonasik merged commit f5820a0 into main Jul 14, 2023
@samsonasik samsonasik deleted the pass-by-ref- branch July 14, 2023 18:31
@staabm
Copy link
Copy Markdown
Contributor

staabm commented Jul 14, 2023

thank you

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jul 14, 2023 •
edited
Loading

@staabm if you want to debug/test, you need to move var_dump($filePaths) after call configurePHPStanNodeScopeResolver() called:

$filePaths = $this->applicationFileProcessor->configurePHPStanNodeScopeResolver($filePaths, $configuration);
+var_dump($filePaths);

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jul 14, 2023

@TomasVotruba it seems make progress bar only got 99% while actually 100% already,

 2966/2973 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░]  99%

Ref https://github.com/rectorphp/rector-src/actions/runs/5557453961/jobs/10151262337#step:11:28

I will cherry-pick progress bar tweak from 88a755b

@samsonasik samsonasik mentioned this pull request Jul 14, 2023
Merged
@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jul 14, 2023

#4516

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jul 14, 2023 •
edited
Loading

@TomasVotruba @staabm it seems for both parallel and non-parallel process; filePaths should be filtered early, so total counts will make always correct, so move this:

rector-src/src/Application/ApplicationFileProcessor.php

Lines 78 to 79 in 9217e0d

// 1. PHPStan has to know about all files too
$filePaths = $this->configurePHPStanNodeScopeResolver($filePaths, $configuration);

before if check

rector-src/src/Application/ApplicationFileProcessor.php

Lines 74 to 75 in 9217e0d

if ($configuration->isParallel()) {

I will try tomorrow, now I need to sleep

@samsonasik samsonasik mentioned this pull request Jul 15, 2023
Merged
@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jul 15, 2023

#4519

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomasVotruba TomasVotruba Awaiting requested review from TomasVotruba

1 more reviewer

@staabm staabm staabm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Worker process files which do not match the configured fileextension

3 participants

@samsonasik @staabm @actions-user