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

Cache result of include_in_build_config and inhibit_warnings #5934

Merged
merged 1 commit into from Sep 27, 2016

Conversation

Projects
None yet
4 participants
@dnkoutso
Contributor

dnkoutso commented Sep 23, 2016

We re-run the profiler against our project and noticed two more big improvements in pod install times.

If you wanted to see how long a project with 10+ targets and 130 pods takes to integrate I've attached the generated log here. You might be able to pick other areas of improvements but we noticed again that pod_target could be caching some of its results again.

I do not believe caching these could cause any harm or throw a pod_target out of sync.

profile.zip (it might take a bit of time to render in Chrome :))

This dropped our pod install time from 1m 8s to about 48seconds.

@dnkoutso dnkoutso force-pushed the dnkoutso:master branch 2 times, most recently from 3e4e76f to 845f2c7 Sep 23, 2016

@@ -8,6 +8,10 @@ To install release candidates run `[sudo] gem install cocoapods --pre`
##### Enhancements
* Cache result of inhibit_warnings and include_in_build_config to speed up pod install.
[Dimitris Koutsogiorgas](https://github.com/dnkoutso)
[#5857](https://github.com/CocoaPods/CocoaPods/pull/5857)

This comment has been minimized.

@orta

orta Sep 25, 2016

Member

This should be 5934 👍

This comment has been minimized.

@dnkoutso

dnkoutso Sep 25, 2016

Contributor

Duh fixed ;)

@orta

What's the feel WRT testing some of this @dantoml ?

@@ -243,13 +251,16 @@ def include_in_build_config?(target_definition, configuration_name)
# @return [Bool]
#
def inhibit_warnings?
return @inhibit_warnings if defined? @inhibit_warnings

This comment has been minimized.

@orta

orta Sep 25, 2016

Member

interesting trick 👍

@dnkoutso dnkoutso force-pushed the dnkoutso:master branch from 845f2c7 to e69572d Sep 25, 2016

@dantoml

This comment has been minimized.

Member

dantoml commented Sep 25, 2016

This looks interesting, I'll need to dig in to the code a bit to see if there are any ways this can be invalidated (not too familiar with it off the top of my head), but provisional lgtm.

@dnkoutso dnkoutso force-pushed the dnkoutso:master branch from e69572d to e833c8c Sep 25, 2016

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Sep 25, 2016

FWIW, we've been running with this change internally for over a week with 0 problems and its similar to #5837

@segiddins

This comment has been minimized.

Member

segiddins commented Sep 26, 2016

👍 lgtm

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Sep 26, 2016

🎉

@dantoml dantoml merged commit 93e847f into CocoaPods:master Sep 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dantoml

This comment has been minimized.

Member

dantoml commented Sep 27, 2016

Thanks @dnkoutso!

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Sep 27, 2016

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment