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

use is_file() over file_exists(), which is faster #724

Merged
merged 2 commits into from
Oct 25, 2021

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Oct 20, 2021

file_exists() is up to 100 times slower

Inspired by https://bugs.php.net/bug.php?id=78285

Comment on lines -42 to -44
if (!file_exists($path)) {
throw new \PHPStan\File\PathNotFoundException($path);
} elseif (is_file($path)) {
Copy link
Contributor Author

@staabm staabm Oct 20, 2021

Choose a reason for hiding this comment

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

this is the only spot where logic was changed within the PR.

I have moved the fast is_file positive-lookup case in the front and moved the less-likely file_exists negative case into the else-if

@staabm staabm changed the title use is_file() over file_exists() which is faster use is_file() over file_exists(), which is faster Oct 20, 2021
@staabm
Copy link
Contributor Author

staabm commented Oct 25, 2021

I was not able to come up with a profile which shows a measurable improvement by this PR.

only thing I can say is, that is_file() and friends are doing way less syscalls and therefore perform better.
in case this is good enough for you as an argument feel free to merge.

in case you wanne play safe, we can close here. its up to you.

@ondrejmirtes
Copy link
Member

The only risk here is that file_exists can return true for directories, but after careful review I don't think we're pasing a directory in any of the changed lines.

@ondrejmirtes ondrejmirtes merged commit a1e2fd9 into phpstan:master Oct 25, 2021
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the is-file branch October 25, 2021 15:44
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.

3 participants