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 parallelisation on projects manifest loading #3092

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

adellibovi
Copy link
Collaborator

Short description πŸ“

This improve performance for manifest with several projects manifest. Currently each project manifest is compiled one by one, even if swiftc could be fast, we still have ~0.5/1s for launching an external process. Under my tests, it went down from 4/5mins to ~60s.

Ideally, I think it would be great if we could compile all the manifest as a single process, (in comparison the same "configuration" using just one project takes ~15/20s) but that would probably require quite a significant change in both the code and Tuist api.

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.

@kwridan kwridan self-requested a review June 21, 2021 16:42
Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Thanks for adding this improvement @adellibovi. If the tests pass, I think we can be confident that this change won't break anything on the user side. Don't forget to update the CHANGELOG before we can merge it :).
The one question that I have is when you said, it'd be great to compile all the manfiests in one process, what do you mean? Is that possible with the compiler? If so, I think it's a good idea to explore that it'd take to get there. Providing users with a great performance I think it's important.

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Awesome thanks @adellibovi!

This should improve cold generation times in many cases, I'm curious to see the effect on warm generation times as that is already an order of magnitude faster (reading json rather than compiling Swift) - will run the benchmark locally.

@kwridan
Copy link
Collaborator

kwridan commented Jun 21, 2021

    Fixture       : workspace_with_50_projects
    Runs          : 5
    Result
        - cold : 4.06s  vs  12.86s (β¬‡οΈŽ -8.80s -68.42%)
        - warm : 0.55s  vs  0.52s (β¬†οΈŽ +0.03s +4.96%)
    Fixture       : workspace_with_100_projects
    Runs          : 5
    Result
        - cold : 6.11s  vs  23.52s (β¬‡οΈŽ -17.41s -74.04%)
        - warm : 0.78s  vs  0.73s (β¬†οΈŽ +0.05s +6.38%)

The fixtures I generated were using fixturegen which generates a workspace with all the project paths pre-defined (best case scenario) where we know all the projects paths upfront. Day to day results will vary as not everyone may be defining all their project paths in a Workspace.swift - but should see some overall improvements.

As we suspect cold generation times gain a huge boost, warm generation times however may get slightly slower due to the parallelism overhead in contrast with the overall loading times (which is already significantly faster). We're talking ~50 ms here so I'm sure there's some rounding errors and is worthwhile given the overall benefit. Something to keep an eye on for future optimisations.

Thanks again!

@fortmarek
Copy link
Member

Ideally, I think it would be great if we could compile all the manifest as a single process, (in comparison the same "configuration" using just one project takes ~15/20s) but that would probably require quite a significant change in both the code and Tuist api.

Indeed, however, for that to work we'd need to revise how we get the JSON representation of a manifest since right now it relies on a single one being emitted per compilation (and then running dump).

One way we could maybe achieve this is by compiling all manifests together and letting them all dump their contents in one go. We'd then have to identify which manifest is which based on their path. Probably worth doing this exploration and seeing how much improvements we might see.

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.

πŸ‘ I think we can merge this, could you just add a log to the CHANGELOG.md?

@adellibovi
Copy link
Collaborator Author

adellibovi commented Jun 22, 2021

The one question that I have is when you said, it'd be great to compile all the manfiests in one process, what do you mean? Is that possible with the compiler? If so, I think it's a good idea to explore that it'd take to get there. Providing users with a great performance I think it's important.

One way we could maybe achieve this is by compiling all manifests together and letting them all dump their contents in one go. We'd then have to identify which manifest is which based on their path. Probably worth doing this exploration and seeing how much improvements we might see.

If I understood it correctly, currently we depend on the top level code execution for dumping the information, so we cannot just "compile" everything, as it will try to run just one file and therefor dump only one file. That's what I meant by "it may require tuist api to change": to find a way to trigger the dump without the need of executing top level code.
Manifest & json parsing should be easy to achieve IMHO, i.e. we could just encode the identifier in the existing TUIST_MANIFEST_START separators.

Anyway I will give it a deeper thought and let you folks up-to-date :)
In the meanwhile I pushed the updated CHANGELOG. Thanks for the review!

@pepicrft pepicrft merged commit 8dcc789 into main Jun 22, 2021
@pepicrft pepicrft deleted the parellel-project-loading branch June 22, 2021 16:30
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

4 participants