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

path list optimizations #9428

Merged
merged 3 commits into from Dec 24, 2019
Merged

path list optimizations #9428

merged 3 commits into from Dec 24, 2019

Conversation

manuyavuz
Copy link
Member

  • Uses Dir.glob instead of Find.find for faster content fetch
  • Does not recalculate file system content for local pods during installation as they won't be changed by cocoapods itself anytime.

Improvements

Previous

bundle exec pod install --clean-install  10.97s user 7.97s system 96% cpu 19.628 total
bundle exec pod install  4.91s user 4.41s system 95% cpu 9.772 total

This branch

bundle exec pod install --clean-install  6.62s user 4.12s system 86% cpu 12.453 total
bundle exec pod install  3.99s user 3.60s system 88% cpu 8.587 total

@manuyavuz
Copy link
Member Author

Partially fixes #9427.

@manuyavuz
Copy link
Member Author

Do we need a changelog here?

@dnkoutso
Copy link
Contributor

Yeah we can add a changelog entry for "Various performance optimizations around file system reads" or something like that so we can know it went through.

@dnkoutso dnkoutso added this to the 1.9.0 milestone Dec 24, 2019
@dnkoutso
Copy link
Contributor

Those are very nice improvement times!

@dnkoutso
Copy link
Contributor

LGTM. Any tests to add here?

@dnkoutso
Copy link
Contributor

oh @manuyavuz can you also target 1-9-stable branch instead since we want this for 1.9.0!

@manuyavuz manuyavuz changed the base branch from master to 1-9-stable December 24, 2019 08:24
@manuyavuz
Copy link
Member Author

@dnkoutso rebased onto 1-9-stable. What is our branch policy, will you cherry pick into master after merge?

@manuyavuz
Copy link
Member Author

For tests, there are existing tests for escaped path logic, but there is no test for refresh_file_accessors logic. There is no test for not cleaning local pods inside pod_dir_cleaner for example.

If we want to have better coverage here, I can spend some time tonight maybe. But I'm not sure if this should go into this PR.

@dnkoutso
Copy link
Contributor

@manuyavuz I take care of merging up to master so we are good for now.

@dnkoutso dnkoutso merged commit 0b5328c into 1-9-stable Dec 24, 2019
@dnkoutso
Copy link
Contributor

I will try this to our large project and see impact as well as ensure no regressions.

@manuyavuz
Copy link
Member Author

Awesome, lmk about results!

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.

None yet

2 participants