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

Speed up SettingsBuilder #1221

Merged
merged 2 commits into from
Jun 16, 2022
Merged

Conversation

jpsim
Copy link
Contributor

@jpsim jpsim commented Jun 13, 2022

It's unnecessary to build up a whole grouped dictionary only to check if all platforms are identical and then immediately discard the dictionary.

Instead we can check if all targets match the first platform, which avoids creating a new dictionary but also allows bailing early as soon as a non-matching platform is found.

Generating a large project (36MB json spec) on an M1 Max machine leads to a ~6% total speedup: 28.48s vs 30.07s.

It's unnecessary to build up a whole grouped dictionary only to check
if all platforms are identical and then immediately discard the
dictionary.

Instead we can check if all targets match the first platform, which
avoids creating a new dictionary but also allows bailing early as soon
as a non-matching platform is found.

Generating a large project (36MB json spec) on an M1 Max machine leads
to a ~6% total speedup: 28.48s vs 30.07s.
Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Cool, thanks @jpsim. Could you please add a changelog entry as well under Next Version.

Given your large amount of targets I imagine there are a wealth of small performance optimisations like this.

@jpsim jpsim requested a review from yonaskolb June 14, 2022 14:22
@jpsim
Copy link
Contributor Author

jpsim commented Jun 14, 2022

Changelog entry added

@jpsim
Copy link
Contributor Author

jpsim commented Jun 14, 2022

Given your large amount of targets I imagine there are a wealth of small performance optimisations like this.

The number one thing we could do to optimise project generation would be to parallelise the source generator.

From the 10s mark to the 45s mark in this trace, the bottleneck is recursively walking the directory tree in SourceGenerator.getSourceChildren(...), which runs single-threaded.

image

I've tried parallelising that work but it's challenging given that the XcodeProj structure that gets built up at that stage uses a lot of reference types that are unsafe to modify from multiple threads. I've tried introducing locks and serial queues to address that, but haven't gotten it to work so far. I also tried to use Swift Concurrency to help parallise this processing stage safely, but the CLI library XcodeGen uses doesn't support async commands, so I gave up on that as well. swift-argument-parser does though, if you wanted to go down that route.

My next attempt would build up the directory tree in memory in a parallelised way and then use that tree for further work that is challenging to parallelise, but I'm not sure how much of a win we'll get from that.

Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Yeah been thinking a move to swift-argument-parser would be good in the long run

@yonaskolb yonaskolb merged commit f65dad7 into yonaskolb:master Jun 16, 2022
@jpsim jpsim deleted the speed-up-settingsbuilder branch June 16, 2022 10:48
@asifmohd
Copy link
Contributor

Thank you for this improvement @jpsim 👍 It has helped with an improvement of 25% for our project ❤️

I've tried introducing locks and serial queues to address that, but haven't gotten it to work so far.

@jpsim Would be amazing if you could try out a build from this MR and share your feedback 🙏

#863

I've been using a build from this branch for the past 2 years, and it has been super useful to save time spent generating projects. On an average I am seeing a 50% improvement in project generation times compared to master.

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.

4 participants