-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Default to ninja for faster builds (mac & linux) #124041
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
Benchmarking showed ninja provides ~8.8% faster builds on macOS (avg 654s vs 718s per build). This change defaults to using ninja when the host OS is macOS. Changes: - eng/build.sh: Default --ninja true on macOS, track explicit user override - eng/native/build-commons.sh: Default -ninja on macOS, add -ninja false opt-out Users can opt out with --ninja false (or -ninja false for native builds).
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.
Pull request overview
This PR improves macOS build performance by defaulting to the Ninja build system instead of Make, achieving approximately 8.8% faster build times (654s vs 718s average). The change is opt-out via --ninja false to maintain flexibility.
Changes:
- Default ninja build system on macOS for improved performance
- Add explicit flag tracking to prevent overriding user preferences
- Update help text to document the new default and opt-out mechanism
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| eng/build.sh | Adds macOS-specific ninja default with tracking to avoid overriding explicit user settings |
| eng/native/build-commons.sh | Sets ninja as default on macOS and enhances -ninja flag to accept true/false values |
|
docs\workflow\requirements\macos-requirements.md needs to be updated - ninja is no longer optional by default after this change. |
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
|
Windows also defaults to ninja. Maybe we should flip the switch: |
Nit: Avoiding negations in names is a generally better UX design. I like |
- Use single useNinja variable in eng/build.sh instead of tracking explicit-ness - Initialize __UseNinja=0 in build-commons.sh, then override to 1 on macOS - Remove __UseNinja=0 initialization from build-runtime.sh and tests/build.sh so the macOS default takes effect - Update macOS requirements docs to reflect ninja is now default
|
I was just talking with @agocke about requiring Ninja on macOS to make it easier to maintain our Swift usage (as CMake only supports Swift with Ninja and XCode) so we don't need to manually invoke the Swift compiler and do our own object file handling. I think this is a great first step in that direction! |
Based on PR feedback, default to using Ninja for native builds on any Unix platform where ninja is installed, rather than only on macOS. This aligns with the Mono subtree behavior and provides faster builds when ninja is available.
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
|
@jkotas @jkoritzinsky can I get your approval? |
|
Do we have some numbers for the build time improvement on Linux? |
I didn't run it b/c there was preference to just turn it on for both. I'll run something just so we know. |
|
Last time we got numbers the improvement was marginal (on the order of a few seconds) but there was an improvement. I think for such a situation a default of "use if available" is reasonable. |
|
It was implemented that way when you first asked, but later reverted due to concerns about build determinism (see #124041 (comment)). I believe build determinism is a separate issue, and Ninja by itself cannot meaningfully affect it: even small differences in toolchain versions or machine environments prevent bit-for-bit identical outputs. Docker largely addresses this on Linux by fixing the environment, but on other platforms there are more fundamental sources of nondeterminism to address, well beyond what Ninja alone can affect. |
|
Ok, @am11 votes for use if installed. @jkoritzinsky you appear to be as well. I don't have a strong opinion, but if I had to pick, I'd choose ninja by default and only opt out explicitly. |
I see 5% improvement for
Yes, I think we should have it on by default for macOS and Linux at least, so that folks do not accidentally forget to install it and get worse build times.
I am fine with off by default (or automatic light up) for other platforms if it helps. |
|
I'm okay with on-by-default as well. I just figured that use-if-available is an easier change to accept if we weren't sure if we wanted to switch. |
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
When -ninja was followed by another flag (e.g. --ninja --subset clr), the parser would consume the next argument as a ninja value and break argument parsing. Now check if $2 starts with '-' before treating it as a true/false value.
Ran on WSL for a 4 hour loop. 12% better there on average. |
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.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
jkotas
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.
Thanks!
|
This broke runtime-libraries enterprise-linux (that was skipped on the PR). #124187 |
Benchmarking showed ninja provides ~8.8% faster builds on macOS (avg 654s vs 718s per build). This change defaults to using ninja for both MacOS and Linux.
Changes:
Users can opt out with --ninja false (or -ninja false for native builds).
Contributes to #54022 (#54022)