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 uses_swift and should_build #5837

Merged
merged 1 commit into from Sep 14, 2016

Conversation

Projects
None yet
3 participants
@dnkoutso
Contributor

dnkoutso commented Sep 10, 2016

We have a large project and our pod install time keeps increasing the more targets and pods we add.

Based on an old profile we did (http://danielribeiro.github.io/braintree_ios/ProfileForCallStackPrinter.html) it seems that during installation a lot of time is spent for checking if a target can be built or if it uses swift.

This small change speeds up our pod install by a lot (from 1m 40s to about 1m and 3s).

@segiddins the only concern is if that value should be invalidated at any point. For example, is there a chance the prepare_command of a pod generate sources that this change will miss?

@dnkoutso dnkoutso force-pushed the dnkoutso:master branch 2 times, most recently from e24b9b7 to 0376f08 Sep 10, 2016

source_files -= file_accessors.flat_map(&:headers)
!source_files.empty?
return @should_build if defined? @should_build
@should_build ||= begin

This comment has been minimized.

@segiddins

segiddins Sep 11, 2016

Member

the ||= is redundant, it will never be used, this code will only ever be executed once per instance

This comment has been minimized.

@dnkoutso

dnkoutso Sep 11, 2016

Contributor

Will fix.

@segiddins

This comment has been minimized.

Member

segiddins commented Sep 11, 2016

Yes, the prepare_command could change this Since the prepare command isn't run via a PodTarget this should be fine

@segiddins

This comment has been minimized.

Member

segiddins commented Sep 11, 2016

What's the estimated savings of this change?

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Sep 11, 2016

I don't have a new profile run to tell you the exact numbers, but as I said in the initial description, it took our pod install time down to 1 minute from 1 minute and ~40 seconds.

In another app it took it down from 23 seconds to 12.

Again this is mostly our environment that has 100+ pods being installed with a lot of files.

@dnkoutso dnkoutso force-pushed the dnkoutso:master branch from 0376f08 to 49f6b10 Sep 11, 2016

@segiddins

This comment has been minimized.

Member

segiddins commented Sep 11, 2016

👍 looks good to me!

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Sep 11, 2016

Awesome to hear on the prepare command! Thank you for verifying this I was just trying to :)

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Sep 14, 2016

Hoping to get this landed for 1.1.0!

@segiddins segiddins merged commit ac9250e into CocoaPods:master Sep 14, 2016

1 check passed

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

This comment has been minimized.

Member

segiddins commented Sep 14, 2016

@dantoml I'm 👍 to back port this if you are

@dantoml

This comment has been minimized.

Member

dantoml commented Sep 14, 2016

@segiddins 👍

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