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

Improve tuist generate speed by caching Swift version fetching #2546

Merged
merged 1 commit into from Feb 22, 2021

Conversation

adellibovi
Copy link
Collaborator

@adellibovi adellibovi commented Feb 19, 2021

Short description 📝

Hello,
Currently every time a Target is generated we try to fetch what Swift version we have on the system.
This is an expensive operation since we need to execute an external shell command.
The impact can be easily seen when having a good number of targets.
In my tests, I've got tuist generate (release mode) to run from 20s to 13s, a nice 40% of improvement!

Update: got similar (or even better) results using tuistbenchmark for an existing fixture:

Fixture       : ios_app_with_tests
Runs          : 4
Result
    - cold : 0.76s  vs  1.14s (⬇︎ -0.39s -33.81%)
    - warm : 0.23s  vs  0.66s (⬇︎ -0.43s -64.98%)

and for a generated fixture: (fixturegen --projects 2 --targets 50 --sources 50):

Fixture       : generated
Runs          : 4
Result
    - cold : 2.19s  vs  5.11s (⬇︎ -2.93s -57.21%)
    - warm : 1.16s  vs  3.75s (⬇︎ -2.59s -69.01%)

Checklist ✅

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

@adellibovi adellibovi force-pushed the improve-tuist-generate-speed branch 2 times, most recently from 36cfbc5 to 3671bd6 Compare February 19, 2021 13:44
Copy link
Collaborator

@luispadron luispadron left a comment

Choose a reason for hiding this comment

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

Nice! Thank you for contributing 🚀

@@ -585,17 +585,23 @@ public final class System: Systeming {
try process.launch()
}

var cachedSwiftVersion: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to add some locks to prevent race conditions when accessing it from multiple threads

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch, done! I noticed we have Atomic property wrapper in Tuist Support, that should do the work :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

To help stress test this you could try generating a fixture with 50 projects :)

Copy link
Member

@fortmarek fortmarek 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 great to see, thanks!

@fortmarek fortmarek merged commit 97d7fc7 into main Feb 22, 2021
@adellibovi adellibovi deleted the improve-tuist-generate-speed branch February 22, 2021 17:16
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

6 participants