Skip to content

Reduce excessive regex use#4950

Merged
samsonasik merged 2 commits intorectorphp:mainfrom
staabm:less-regex
Sep 9, 2023
Merged

Reduce excessive regex use#4950
samsonasik merged 2 commits intorectorphp:mainfrom
staabm:less-regex

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented Sep 9, 2023

see rectorphp/rector#8175 (comment)


blackfire suggest a reduction in memory usage by 67% per worker when running rector on the mautic codebase

grafik

@staabm staabm requested a review from TomasVotruba as a code owner September 9, 2023 11:39
@samsonasik
Copy link
Member

Thank you @staabm

@samsonasik samsonasik merged commit ee6e3e0 into rectorphp:main Sep 9, 2023
@staabm staabm deleted the less-regex branch September 9, 2023 11:44
Comment on lines +201 to +209
$containsWildcard = false;
foreach(self::REGEX_WILDCARD_CHARS as $char) {
if (str_contains($desiredName, $char)) {
$containsWildcard = true;
break;
}
}

if ($containsWildcard) {
Copy link
Member

Choose a reason for hiding this comment

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

When I see this method, I wonder if it should be doing regex check at all. It's a bit magical behavior from isName() and in 99.9 % cases never used.

Instead there should be matchName() method dedicated for this. I've run tests without it and it only reports ~10 fixtures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would be great if we could reduce regex usage to only the necessary places.

after this change it no longer shows up in the memory focused profiles, but the method here is still the top1 method regarding CPU use - even after the PR

grafik

Copy link
Member

Choose a reason for hiding this comment

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

Could you give the separated method a try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you could give me a hint how to get rector running again :)

rectorphp/rector#8191

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