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

[deliver] Implement parallel screenshot downloads #16654

Merged
merged 1 commit into from
Jul 8, 2020

Conversation

CognitiveDisson
Copy link
Contributor

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

I am working on an application that supports many languages. Therefore, loading and unloading screenshots for all of them takes a lot of time. To get started, I would like to add a parallel download of screenshots. I know that Fastlane is not designed for parallel operations, but in this case I think this is a suitable solution.

Description

I added the creation of a thread for downloading screenshots for each language, this should be the optimal solution (the optimal number of threads and file descriptors) and should not create collisions.

Testing Steps

Execute fastlane deliver download_screenshots on the current release version of Fastlane and on version from the current branch.

Averaged local measurement results:
Hardware: MacBook Pro (15-inch, 2019) 2.4 GHz Intel Core i9 32 GB 2400 MHz DDR4
Input: 31 language * 4 devices * 9 screenshots = 1116 images 421.6 MB
Connection: WiFi 46.9 Mbps

Result before: ~ 375s
Result after: ~ 99s

@googlebot

This comment has been minimized.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@CognitiveDisson

This comment has been minimized.

@CognitiveDisson CognitiveDisson changed the title Implement parallel screenshot downloads [deliver] Implement parallel screenshot downloads Jun 27, 2020
@janpio
Copy link
Member

janpio commented Jun 28, 2020

Wow, this changes the game quite a lot.

Are you sure this would not have any possible negative effects? Is it safe to just spawn that many threads (for some apps this will be quite a lot, right?)

@CognitiveDisson
Copy link
Contributor Author

CognitiveDisson commented Jun 30, 2020

Hi @janpio!

Are you sure this would not have any possible negative effects?

The only negative effect that I see is that the processor and network channel will be more busy. The processor will be more loaded. And network failures with a weak connection are slightly more likely.

Also on a single-core processor, this will slightly slow down the download due to additional resources for creating threads.
But I think that these are insignificant minuses, since I think that now a small patch of developers uses a single-core processor and dialup Internet 😅

The number of images in the store, the size of the images, and the number of languages supported are limited. Apple supports 40 localizations, 10 screenshots per device, 4 types of devices, 1600 images in total.

On a multi-core processor, creating threads, even if there are more threads than cores, will not lead to negative overhead. I create threads according to the number of localizations, maximum 40, this is not a lot. So it should be fine

@janpio janpio requested a review from joshdholtz June 30, 2020 17:06
@joshdholtz
Copy link
Member

@CognitiveDisson @janpio This is going to conflict with changes I have going on in the 2.150.0 branch. Can we make this an issue to implement later? 😇

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

@CognitiveDisson There are some conflicting changes now due to the recent App Store Connect API updates. Do you mind rebasing and fixing this? 😇

@CognitiveDisson
Copy link
Contributor Author

@joshdholtz Of course, I will do it soon. Thanks for letting me know.

@CognitiveDisson
Copy link
Contributor Author

@joshdholtz Done and checked

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This is so quick! Thanks for rebasing this 😊 Really appreciate it! ❤️

@joshdholtz joshdholtz merged commit 9a43441 into fastlane:master Jul 8, 2020
@fastlane-bot
Copy link

Hey @CognitiveDisson 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

Copy link

@fastlane-bot fastlane-bot left a comment

Choose a reason for hiding this comment

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

Congratulations! 🎉 This was released as part of fastlane 2.151.2 🚀

minuscorp pushed a commit to minuscorp/fastlane that referenced this pull request Jul 18, 2020
@fastlane fastlane locked and limited conversation to collaborators Sep 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants