Skip to content

Fix phpstan/phpstan#14073: Performance - list the most-often-analysed trait files in -vvv (DiagnoseExtension)#5395

Merged
ondrejmirtes merged 13 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-ihwryz1
Apr 8, 2026
Merged

Fix phpstan/phpstan#14073: Performance - list the most-often-analysed trait files in -vvv (DiagnoseExtension)#5395
ondrejmirtes merged 13 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-ihwryz1

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

@phpstan-bot phpstan-bot commented Apr 4, 2026

Implements the feature requested in phpstan/phpstan#14073: when running PHPStan with -vvv, the diagnostic output now includes the top 5 most-often-analysed files (typically trait files). This helps detect performance issues like the one found in Shopware where trait files were analysed an excessive number of times.

Fixes phpstan/phpstan#14073


running this PR on wordpress-develop

➜  wordpress-develop git:(trunk) ../../bin/phpstan analyze src -vvv
Note: Using configuration file /Users/staabm/workspace/phpstan-src/tmp/wordpress-develop/phpstan.neon.dist.
Result cache not used because the metadata do not match: phpstanVersion, projectConfig
 1512/1512 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% 21 secs/21 secs

Result cache is saved.

                                                                                                                        
 [OK] No errors                                                                                                         
                                                                                                                        

PHP runtime version: 8.3.30
PHP version for analysis: 7.4-8.5 (from config)

PHPStan version: 2.1.x-dev@a857004
PHPStan running from:
../../bin/phpstan

Extension installer: Not installed

Discovered Composer project root:
/Users/staabm/workspace/phpstan-src/tmp/wordpress-develop

Most often analysed files:
  src/wp-includes/php-ai-client/src/Providers/Http/Traits/WithHttpTransporterTrait.php: 4 times
  src/wp-includes/php-ai-client/src/Providers/Http/Traits/WithRequestAuthenticationTrait.php: 3 times
  src/wp-includes/php-ai-client/src/Common/Traits/WithDataCachingTrait.php: 2 times

Parallel processing scheduler:
# of detected CPU cores:   14
# of analysed files:       1512
# of jobs:                 76
# of spawned processes:    8

Elapsed time: 21.36 seconds
Used memory: 3.98 GB

@staabm staabm requested a review from VincentLanglet April 4, 2026 15:52
@VincentLanglet
Copy link
Copy Markdown
Contributor

I just wonder if a new feature like this should be approved by ondrej @staabm.

Also if we should directly count in the addProcessedFiles to reduce the memory consuption in case there is lot of duplicated files.

@staabm
Copy link
Copy Markdown
Contributor

staabm commented Apr 4, 2026

Also if we should directly count in the addProcessedFiles to reduce the memory consuption in case there is lot of duplicated files.

I think we cannot save much because the path strings are already allocated at the analyzer level and collecting them in an array shouldn't add much more.

@staabm staabm force-pushed the create-pull-request/patch-ihwryz1 branch from bbba77e to cd1c978 Compare April 4, 2026 19:35
@staabm staabm requested a review from ondrejmirtes April 4, 2026 19:35
use function sprintf;

#[AutowiredService]
final class ProcessedFilesDiagnoseExtension implements DiagnoseExtension
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

instead of having this separate DiagnoseExtension the logic could be merged into PHPStanDiagnoseExtension

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure if it was meant to the bot, but I don't think it handle this feedback since you approved the PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah that was intentional

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The make tests background task also completed successfully. All checks pass.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The changes are already pushed to the remote. The commit 023dddf0b contains exactly the same changes I made — merging ProcessedFilesDiagnoseExtension into PHPStanDiagnoseExtension, updating the service config, and removing the separate class and its test.

The review feedback from staabm has been addressed:

  • Removed the separate ProcessedFilesDiagnoseExtension class
  • Merged the "most often analysed files" diagnostic output into PHPStanDiagnoseExtension
  • Added ProcessedFilesCollector and RelativePathHelper as constructor dependencies
  • Updated conf/services.neon to pass simpleRelativePathHelper
  • Removed the now-unnecessary test file
  • Both make phpstan and make tests pass cleanly

@VincentLanglet
Copy link
Copy Markdown
Contributor

I think it handles the comment because of me @staabm #5395 (comment)
I think that answering to a review retrigger the bot (even if the initial message was an approval)

Do you want to revert it then?

@staabm
Copy link
Copy Markdown
Contributor

staabm commented Apr 5, 2026

Lets wait for ondrejs opinion

@staabm staabm force-pushed the create-pull-request/patch-ihwryz1 branch from 023dddf to 38aef23 Compare April 5, 2026 17:33
staabm and others added 10 commits April 8, 2026 09:53
- Added ProcessedFilesCollector service to accumulate processed files during analysis
- Added TraitAnalysisDiagnoseExtension to print top 5 most-analysed files in -vvv output
- Modified WorkerCommand to send processedFiles in parallel worker JSON responses
- Modified ParallelAnalyser to pass processedFiles through postFileCallback
- Modified AnalyseApplication to collect processedFiles in both debug and non-debug modes
- Added unit tests for ProcessedFilesCollector and TraitAnalysisDiagnoseExtension
Address review feedback: instead of having a separate DiagnoseExtension
for processed files, merge the logic into the existing PHPStanDiagnoseExtension.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@staabm staabm force-pushed the create-pull-request/patch-ihwryz1 branch from e2b93b3 to 91794e0 Compare April 8, 2026 07:53
/**
* @param list<string> $files
*/
public function addProcessedFiles(array $files): void
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't like this class is called a Collector, and I don't like the mutable design. Also DI services should be stateless, so this is a big no-no. This data instead should be carried from Analyser and ParallelAnalyser (so in AnalyserResult as DTO) all the way to runDiagnoseExtensions in AnalyseCommand and to PHPStanDiagnoseExtension.

It's okay to remove implements DiagnoseExtension from PHPStanDiagnoseExtension in order to change the print signature.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

All done. Here's a summary of the changes made to address ondrejmirtes' review:

Problem: ProcessedFilesCollector was a mutable DI service (stateful singleton), which violates the principle that DI services should be stateless.

Solution: Carry processedFiles data through AnalyserResult as an immutable DTO field instead:

  1. AnalyserResult — Added processedFiles field (with [] default) and getProcessedFiles() getter
  2. Analyser — Accumulates processedFiles from each FileAnalyserResult and includes in the returned AnalyserResult
  3. ParallelAnalyser — Accumulates processedFiles from worker JSON messages and includes in the returned AnalyserResult
  4. AnalyseApplication — Removed ProcessedFilesCollector dependency. Changed analyse() to return array{AnalysisResult, list<string>}. Extracts processedFiles from intermediateAnalyserResult before cache processing
  5. AnalyseCommand — Destructures the new return type. Passes processedFiles to runDiagnoseExtensions()
  6. PHPStanDiagnoseExtension — Removed implements DiagnoseExtension. Changed print() to accept array $processedFiles. Moved top-files computation (from deleted ProcessedFilesCollector) into a private method
  7. Deleted ProcessedFilesCollector class and its test
  8. All intermediate AnalyserResult transformations (finalizer, cache manager, switchTmpFile) carry processedFiles through

…AnalyserResult DTO

Address review feedback: DI services should be stateless. Instead of
accumulating processedFiles in a mutable singleton, the data now flows
through AnalyserResult as an immutable DTO field from Analyser and
ParallelAnalyser, through AnalyseApplication, to AnalyseCommand's
runDiagnoseExtensions and PHPStanDiagnoseExtension.

- Add processedFiles field to AnalyserResult
- Accumulate processedFiles in Analyser and ParallelAnalyser
- Return processedFiles alongside AnalysisResult from AnalyseApplication
- Remove implements DiagnoseExtension from PHPStanDiagnoseExtension
- Pass processedFiles explicitly to PHPStanDiagnoseExtension::print()
- Move top-files computation into PHPStanDiagnoseExtension
- Delete ProcessedFilesCollector and its test

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sResult again

Move processedFiles from a separate array return value into the AnalysisResult
DTO so that AnalyseApplication::analyse() returns AnalysisResult directly
instead of array{AnalysisResult, list<string>}.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

After doing these changes please verify it again against wordpress-develop, if it outputs the traits still.

Also verify if it outputs them with -vvv both with and without --debug (parallel and non-parallel).

private array $exportedNodes,
private bool $reachedInternalErrorsCountLimit,
private int $peakMemoryUsageBytes,
private array $processedFiles = [],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need for this to be optional

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The background test run also completed successfully (exit code 0). All changes are pushed.

if (!$debug) {
$preFileCallback = null;
$postFileCallback = static function (int $step) use ($errorOutput): void {
$postFileCallback = static function (int $step, array $processedFiles) use ($errorOutput): void {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need to add the parameter here if it's not used

…ack parameter

Address review feedback:
- Remove default value from AnalyserResult::$processedFiles, add explicit
  processedFiles: [] at all construction sites
- Remove unused $processedFiles parameter from non-debug postFileCallback
- Revert postFileCallback type annotations to optional second parameter

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ondrejmirtes ondrejmirtes force-pushed the create-pull-request/patch-ihwryz1 branch 2 times, most recently from 78ac06a to f2db6d1 Compare April 8, 2026 16:59
@ondrejmirtes ondrejmirtes merged commit 50723e6 into phpstan:2.1.x Apr 8, 2026
1305 of 1306 checks passed
@ondrejmirtes ondrejmirtes deleted the create-pull-request/patch-ihwryz1 branch April 8, 2026 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants