Skip to content

Improve array_search type specifier performance #1818

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
Oct 11, 2022

Conversation

rvanvelzen
Copy link
Contributor

Mostly resolves phpstan/phpstan#8147. The added integration test case went down from a ~minute to ~10 seconds locally.

@rvanvelzen rvanvelzen force-pushed the array-search-type-spec-perf branch from c58f75d to 38aa5ee Compare October 11, 2022 12:29
@herndlm
Copy link
Contributor

herndlm commented Oct 11, 2022

haha, nice one

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ondrejmirtes Ondřej Mirtes
@rvanvelzen rvanvelzen force-pushed the array-search-type-spec-perf branch from 38aa5ee to 943ded7 Compare October 11, 2022 14:02
@ondrejmirtes
Copy link
Member

Something's fishy here. Without the change in src, the test on my machine runs in 6 seconds. With the change, it runs in less than 2 seconds.

$ vendor/bin/phpunit tests/PHPStan/Analyser/AnalyserIntegrationTest.php --filter testBug8147
PHPUnit 9.5.23 #StandWithUkraine

Warning:       No code coverage driver available

.                                                                   1 / 1 (100%)

Time: 00:01.874, Memory: 76.00 MB

OK (1 test, 1 assertion)

Sure, 3x improvement is nice, but the numbers are nowhere near "~minute to ~10 seconds".

Maybe something change meanwhile that also optimized this?

@ondrejmirtes ondrejmirtes merged commit 9bf5694 into phpstan:1.8.x Oct 11, 2022
@ondrejmirtes
Copy link
Member

Thank you.

@rvanvelzen
Copy link
Contributor Author

I did reduce the test case a bit afterwards, which obviously reduces the runtime a bit. My machine was also quite busy which could have skewed the numbers a bit.

@ondrejmirtes
Copy link
Member

Makes sense 😊

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

3 participants