-
-
Notifications
You must be signed in to change notification settings - Fork 590
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
The fixtures I generated were using 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! |
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. |
There was a problem hiding this 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
?
f734606
to
64f38b7
Compare
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. Anyway I will give it a deeper thought and let you folks up-to-date :) |
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 ✅
CHANGELOG.md
has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.