Skip to content

Reduce method calls in ExpressionTypeHolder #3859

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

Merged
merged 1 commit into from
Mar 7, 2025

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 7, 2025

while looking into phpstan/phpstan#12647 I noticed ExpressionTypeHolder->and is on a very hot path.

grafik

lets micro optimize by remove unnecessary method calls in this path

before

➜  PhpSpreadsheet git:(master) ✗ hyperfine 'php ../phpstan-src/bin/phpstan analyze src/PhpSpreadsheet/Reader/Xlsx.php --debug' # before
Benchmark 1: php ../phpstan-src/bin/phpstan analyze src/PhpSpreadsheet/Reader/Xlsx.php --debug
  Time (mean ± σ):      8.948 s ±  0.020 s    [User: 8.774 s, System: 0.170 s]
  Range (min … max):    8.913 s …  8.975 s    10 runs

after

➜  PhpSpreadsheet git:(master) ✗ hyperfine 'php ../phpstan-src/bin/phpstan analyze src/PhpSpreadsheet/Reader/Xlsx.php --debug' # after
Benchmark 1: php ../phpstan-src/bin/phpstan analyze src/PhpSpreadsheet/Reader/Xlsx.php --debug
  Time (mean ± σ):      8.775 s ±  0.011 s    [User: 8.608 s, System: 0.163 s]
  Range (min … max):    8.751 s …  8.790 s    10 runs

@ondrejmirtes
Copy link
Member

Usually we can achieve a much better optimization by reducing the number of calls, not optimizating how fast it's going to run when it's called 2 million times. Can you please investigate why is it called so often? Usually there's some other optimization to be had because of huge combinatorial explosion in types or something like that.

@staabm
Copy link
Contributor Author

staabm commented Mar 7, 2025

I am still looking into the case, but I think this method is called so often that the PR is still worth it - even for a lot of other profiles with big unions.
(in addition it helps getting better profiles with blackfire, because the profiler is very sensitive to function/method calls)

its 50-100ms on a single file, so it easily adds up when analying projects with lots of files

@ondrejmirtes ondrejmirtes merged commit 69369f4 into phpstan:2.1.x Mar 7, 2025
413 of 418 checks passed
@ondrejmirtes
Copy link
Member

Thank you! But we should still investigate why is it called so many times, and avoid that :)

@staabm staabm deleted the less-fn branch March 7, 2025 09:10
@staabm
Copy link
Contributor Author

staabm commented Mar 7, 2025

thank you.

debugged the src/PhpSpreadsheet/Reader/Xlsx.php case further:

there is one union involved with more then 30 elements, which is resolved from the phpdoc ZipArchive::ER_* (from phpstan core signatures for ZipArchive).

the 2nd biggest union has 8 elements and is resolved from this phpdoc
https://github.com/PHPOffice/PhpSpreadsheet/blob/3ef8ec4b1a095f700412c6e44f15b3eaedae23fe/src/PhpSpreadsheet/Worksheet/Worksheet.php#L2713

-> there is no really huge union involved it seems. we spent most of the time with merging scope objects

profile can be seen here: https://blackfire.io/profiles/0293ba31-80fc-403f-9452-bfda15b7c7c7/graph

-> I am still investigating

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.

None yet

2 participants