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

Optimise post rectors #6240

Merged
merged 6 commits into from
Aug 22, 2024

Conversation

carlos-granados
Copy link
Contributor

When I was investigating rectorphp/rector#8793 yesterday I noticed that the NameImportingPostRector post rector could be optimised because it was unnecessarily calculating the use statements for every node so I decided to add a fix. I took the opportunity to look at the other post rectors and found some other possible optimisations. They are small optimisations, nothing really very noticeable, but, as they say, every little helps!

See the PR comments for more info

@carlos-granados
Copy link
Contributor Author

@samsonasik code updated

@TomasVotruba
Copy link
Member

Thanks for the improvement 👍

Could you share few time measurements, as on the other PR?

@carlos-granados
Copy link
Contributor Author

@TomasVotruba time improvements are not really noticeable, I see the same timings more or less with the previous version and this one (the results vary a little but they vary equally in both versions). It is more a question of trying to be optimal overall so that all these little increments don't add up

@TomasVotruba
Copy link
Member

We're adding a state to readonly service here, which I don't see as a good practise.
I'd prefer to have measurable improvement, to balance the added complexity. We've added few "optimizations" in the past, but they added more complexity than real value so they were removed later.

@carlos-granados
Copy link
Contributor Author

I have tested this without using parallel, which gives more stable results that don't vary too much and I can see numbers similar to these:
Without the optimizations:
bin/rector --clear-cache 97.41s user 2.47s system 98% cpu 1:41.25 total
With the optimizations:
bin/rector --clear-cache 97.26s user 2.41s system 98% cpu 1:41.06 total
This represents an improvement of around 0.3%, so not very significant.
I would say that we should probably keep the changes to UnusedImportRemovingPostRector and UseAddingPostRector as they don't really add more complexity and I'll let you decide about the other two

@TomasVotruba
Copy link
Member

Thanks for checking 👍

I would say that we should probably keep the changes to UnusedImportRemovingPostRector and UseAddingPostRector as they don't really add more complexity and I'll let you decide about the other two

You're right. Let's ship this. Could you rebase the PR?

@carlos-granados
Copy link
Contributor Author

@TomasVotruba rebased

@TomasVotruba
Copy link
Member

Thank you 🙏

@TomasVotruba TomasVotruba merged commit e3ad355 into rectorphp:main Aug 22, 2024
36 checks passed
@carlos-granados carlos-granados deleted the optimise-post-rectors branch August 22, 2024 11:41
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