Skip to content

[Renaming] Improve performance of RenamePropertyRector#3698

Merged
samsonasik merged 3 commits intomainfrom
move-before-loop
Apr 28, 2023
Merged

[Renaming] Improve performance of RenamePropertyRector#3698
samsonasik merged 3 commits intomainfrom
move-before-loop

Conversation

@samsonasik
Copy link
Copy Markdown
Member

By move get property fetch type before loop.

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.

continue;
}

$nodeVarType = $this->nodeTypeResolver->getType($propertyFetch->var);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Before this PR the expensive type-resolving was only needed after the cheap name checks succeeded.

I think instead we should leave the check in the loop but do it only after the classlike instanceof check.

Do you have a concrete real world example you are optimizing for?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't have, probably isset variable inside loop is better..., wdyt?

Copy link
Copy Markdown
Contributor

@staabm staabm Apr 28, 2023

Choose a reason for hiding this comment

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

you could null it before the loop and onyl lazy init it when its still null (and only after the class like instanceof check)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok, Updated 👍

@samsonasik samsonasik enabled auto-merge (squash) April 28, 2023 10:25
@samsonasik samsonasik merged commit 7d16e3d into main Apr 28, 2023
@samsonasik samsonasik deleted the move-before-loop branch April 28, 2023 10:26
samsonasik added a commit that referenced this pull request May 8, 2023
* [Renaming] Improve performance of RenamePropertyRector

* early null assign

* [ci-review] Rector Rectify

---------

Co-authored-by: GitHub Action <actions@github.com>
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