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

Add cache for un-changed files #3118

Merged
merged 2 commits into from
Apr 20, 2020
Merged

Add cache for un-changed files #3118

merged 2 commits into from
Apr 20, 2020

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Apr 1, 2020

Depends on #3197

Enable in Config

# rector.yaml
parameters:
    is_cache_enabled: true # false by default

  • add feature flag, disabled by default for testing

Without cache (~ 18 seconds on 200 files)

wihtout_cache

With cache (~ 1 second on 200 files)

with_cache

Ref #3110

Notes

How to approach rules, that needs more scope for analysis, like:

  • PrivatizeLocalOnlyMethodRector
  • RemoveUnusedClassConstantRector

etc. Extend dependency files resolver? Probably best would be:

@TomasVotruba TomasVotruba force-pushed the caching-result branch 7 times, most recently from 8e4fcfa to d072b7c Compare April 1, 2020 19:32
@TomasVotruba TomasVotruba changed the title add Cache Add cache for un-changed files Apr 1, 2020
@ruudboon
Copy link
Contributor

ruudboon commented Apr 1, 2020

Nice work!

Copy link
Contributor

@ruudboon ruudboon left a comment

Choose a reason for hiding this comment

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

Lgtm

@bendavies
Copy link
Contributor

bendavies commented Apr 1, 2020

phpstan and psalm both do dependency tracking for caching, which sounds like you want to do.

@TomasVotruba
Copy link
Member Author

@bendavies This PR uses part of PHPStan dependency resolver. It works quite well.

Rector just sometimes needs full scope of the application, e.g. for unused public class method. For that I came up with interface that needs full cache clear. I wonder if that can be done in some better way. Ideas?

@Aerendir
Copy link
Contributor

Aerendir commented Apr 1, 2020

Wow, great work Tomas!

I will try to understand the implementation, but in those days it is really complex for me due to Covid limitations: I'm forced at home with my wife and my three years old son: it is really really complex to work...

@TomasVotruba
Copy link
Member Author

@Aerendir Oh, I can't imagine how you're feeling while coding in this environment. I wish we all survive this in both physical and mental health. I sometime go for a run in the park or coding there. It helps me to lay of some steam from lockdown. Also hangouting or calling with friends. If you want one, we can do :)

Anyway, if you find something you don't understand, just go for it and write a comment. It's a first draft and I believe most of it can be written much better way. That's why appreciate feedback so much.

@TomasVotruba TomasVotruba force-pushed the caching-result branch 7 times, most recently from 4192638 to 1a7c7cc Compare April 2, 2020 10:30
@TomasVotruba TomasVotruba force-pushed the caching-result branch 8 times, most recently from 60a882b to 82e092c Compare April 19, 2020 23:40
@TomasVotruba TomasVotruba force-pushed the caching-result branch 4 times, most recently from 6bc5699 to da29054 Compare April 20, 2020 07:34
@TomasVotruba
Copy link
Member Author

TomasVotruba commented Apr 20, 2020

Let's 🚢 it!

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.

4 participants