Skip to content

Optimize path related utils on windows#2068

Merged
ondrejmirtes merged 6 commits intophpstan:1.9.xfrom
staabm:fast-normalize
Dec 10, 2022
Merged

Optimize path related utils on windows#2068
ondrejmirtes merged 6 commits intophpstan:1.9.xfrom
staabm:fast-normalize

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented Dec 9, 2022 •
edited
Loading

the changes mainly prevent excessive use of PCRE on windows. this leads to a small performance improvement of ~3%.

more suprising is blackfire reporting a 56% improvement on memory usage (830 MB instead of 1.9 GB).

grafik

tested on windows 11

php -v
PHP 8.1.2 (cli) (built: Jan 19 2022 10:13:52) (NTS Visual C++ 2019 x64)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies
    with blackfire v1.76.0~win-x64-non_zts81, https://blackfire.io, by Blackfire

staabm
staabm commented Dec 9, 2022
View reviewed changes
src/Type/FileTypeMapper.php Outdated
Copy link
Contributor Author

@staabm staabm Dec 9, 2022

Choose a reason for hiding this comment

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

moved this normalization below the early return, to prevent unnecessary work

staabm
staabm commented Dec 9, 2022
View reviewed changes
src/File/FileHelper.php Outdated
Copy link
Contributor Author

@staabm staabm Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

on windows we now take this fast path and prevent PCRE usage for regular paths starting with C:\\

Copy link
Member

@ondrejmirtes ondrejmirtes Dec 9, 2022

Choose a reason for hiding this comment

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

What about other drive letters?

Copy link
Contributor Author

@staabm staabm Dec 9, 2022

Choose a reason for hiding this comment

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

are you "ok" with allowing any uppercase character?

Copy link
Member

@ondrejmirtes ondrejmirtes Dec 9, 2022

Choose a reason for hiding this comment

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

Yeah, but I feel like there has to be a better way other than comparing letters as if they were numbers :)

Copy link
Contributor Author

@staabm staabm Dec 9, 2022

Choose a reason for hiding this comment

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

wdyt about using ord()? :)

to be honest, I don't like both ways. using regex would work, but we wanne be fast, right? :)

Copy link
Contributor Author

@staabm staabm Dec 9, 2022

Choose a reason for hiding this comment

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

ok, another idea. now using range()

@staabm staabm marked this pull request as ready for review December 9, 2022 15:21
@phpstan-bot
Copy link
Collaborator

phpstan-bot commented Dec 9, 2022

This pull request has been marked as ready for review.

ondrejmirtes
ondrejmirtes requested changes Dec 9, 2022
View reviewed changes
src/File/FileHelper.php Outdated
Copy link
Member

@ondrejmirtes ondrejmirtes Dec 9, 2022

Choose a reason for hiding this comment

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

What about other drive letters?

@staabm staabm force-pushed the fast-normalize branch 4 times, most recently from e8120e9 to da764b1 Compare December 10, 2022 09:16
@staabm
Copy link
Contributor Author

staabm commented Dec 10, 2022 •
edited
Loading

@ondrejmirtes any reasons this was not merged yet? the memory improvement is even more valuable then the time improvement IMO (less likely the process needs to go to swap)

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Dec 10, 2022

I think you're overthinking it - what about not checking the letter at all and just checking that 2nd and 3rd character is :\ ?

@staabm
Copy link
Contributor Author

staabm commented Dec 10, 2022

I am fine with that, too. here we go.

@ondrejmirtes ondrejmirtes merged commit d00f0de into phpstan:1.9.x Dec 10, 2022
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Dec 10, 2022

Thank you!

@staabm staabm deleted the fast-normalize branch December 10, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@staabm @phpstan-bot @ondrejmirtes