-
Notifications
You must be signed in to change notification settings - Fork 115
Calamari Parallelise Build, Restore Publish & Package #1497
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
Conversation
1be4abe to
ee0468f
Compare
ee0468f to
e3664c5
Compare
APErebus
left a comment
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.
This all looks good to me!
I appreciate the validation you've done to make sure it's producing the same packages.
build/Build.cs
Outdated
| return; | ||
| } | ||
|
|
||
| var projectName = calamariPackageMetadata.Project?.Name ?? "UnknownProject"; |
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.
would it be better to just throw here instead of the "UnknownProject"?
Do we ever have a null name here?
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.
Updated to throw an exception, projects always have a name.
build/Build.cs
Outdated
|
|
||
| var projectName = calamariPackageMetadata.Project?.Name ?? "UnknownProject"; | ||
| var projectSemaphore = semaphores.GetOrAdd(projectName, _ => new SemaphoreSlim(1, 1)); | ||
| var architectureSemaphore = semaphores.GetOrAdd(calamariPackageMetadata.Architecture ?? "UnknownArchitecture", _ => new SemaphoreSlim(1, 1)); |
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.
Same here...
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.
Netfx doesn't have a runtime specified, that's current behaviour called out above, I've added another comment for context. In this case we're only using the UnknownArchitecture as a semaphore key so functionally this is blocking parallel netfx builds and the UnknownArchitecture is not passed through to any Build/Restore/Publish calls.
// for NetFx target frameworks, we use "netfx" as the architecture, and ignore defined runtime identifiers
This PR brings the average Build & Compile time for Calamari down from ~21 minutes to ~14 minutes. About half of this improvement is from using the PackInParallel behaviour by default and the other half is from parallelising parts of Restore, Build and Publish for Calamari Flavours. In general the issues I ran into are summarised here, we can't just make the DotnetPublish with Restore & Build enabled parallel, even on a per project or framework basis. This change separates out the Build, Restore and Publish stages and makes them parallel where possible. Because there's a lot of File IO operations, this has limits at a relatively low level of parallelism, in general 3/4 processes seems to give the best run time.
Server PR with E2E tests using Calamari Consolidated from this PR
Build times
Bundles are reconstructed as expected in Server
Index.json architectures for each calamari flavour are the same