Skip to content

[build-script] Add an option to build the Foundation tests in another mode #78390

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

finagolfin
Copy link
Member

@finagolfin finagolfin commented Dec 31, 2024

Use it in the linux CI presets to set them to Debug mode and speed up the linux CI, plus add a new preset which keeps building them in Release mode.

I was looking at a passing linux CI run and saw the log timings at the end: it takes longer to build and test the swift-foundation repos than to compile all 7k+ mostly C++ files in LLVM!

--- Build Script Analyzer ---
Build Script Log: /home/build-user/build/.build_script_log
Build Percentage 	 Build Duration (sec) 	 Build Phase
================ 	 ==================== 	 ===========
9.2%              	 1132.94               	 Running tests for foundationtests
9.1%              	 1120.57               	 linux-x86_64-swift-build
9.0%              	 1104.2                	 Building llvm
7.2%              	 878.84                	 Running tests for swiftfoundationtests
6.5%              	 796.81                	 Running tests for swiftpm
5.6%              	 684.7                 	 Building swiftpm
5.5%              	 667.92                	 linux-x86_64-swift-test
4.9%              	 597.64

Looking at the log, building swift-foundation in release mode takes a long time, so let's see if changing it to debug mode helps. Some background - the Foundation repos are built twice on the linux CI: once by CMake, which is the version installed in the toolchain, then a second time by SwiftPM purely for testing.

This pull only affects that second SwiftPM build for testing, which is not shipped in the final toolchain but thrown away. @jmschonfeld and @shahmishal, let me know what you think.

@finagolfin
Copy link
Member Author

@swift-ci please test

@finagolfin
Copy link
Member Author

Appears to have worked, shaving 25 mins off the Foundation build time for the tests in the linux CI:

2.5%              	 269.32                	 Running tests for foundationtests
2.1%              	 228.05                	 Building swiftdriver
1.8%              	 197.15                	 Running tests for swiftfoundationtests

The only issue I see with merging is that this pull now ignores the build flag to build and test everything in release mode on the linux CI. I doubt anybody cares about that, but if that's a concern, I can add a new build-script flag --debug-foundation-tests, similar to the current --debug-foundation flag, and set it for the linux preset, so that this testing configuration is explicitly overriden there.

Mishal and Jeremy, just let me know what you prefer.

@jmschonfeld
Copy link
Contributor

I don't think this is necessarily a bad change to make (I don't think our tests would behave any differently in debug/release mode and having debug asserts enabled during testing might help reveal more potential issues) but I'm not certain about targeting Foundation specifically with this change. This configuration was copied from the SwiftPM configuration which also builds and runs its tests in release mode. I see that the SwiftPM tests in the most recent run with this change are taking around just as long as the swift foundation tests were previously taking:

Build Percentage 	 Build Duration (sec) 	 Build Phase
================ 	 ==================== 	 ===========
10.4%             	 1119.0                	 linux-x86_64-swift-build
10.3%             	 1102.03               	 Building llvm
7.5%              	 802.46                	 Running tests for swiftpm
6.4%              	 683.71                	 Building swiftpm

It seems reasonable to try to be consistent here - have you discussed the same issue with the SwiftPM folks (cc @dschaefer2). It also looks like the swift driver builds/runs tests in release mode, however those don't take nearly as long. Just want to make sure we don't end up in a world where we've applied this change piecemeal to a handful of projects that doesn't leave a clear story as to when we should be testing in release mode and when we should be testing in debug mode.

It also might be worth looking at a breakdown of what is taking so long in this foundation build, I suspect a lot of it is likely coming from the release build of swift-syntax that swift-foundation/swift-corelibs-foundation (built via SwiftPM) depends on

@finagolfin
Copy link
Member Author

I'm not certain about targeting Foundation specifically with this change.

The reason to target Foundation on linux specifically is that most repos build everything at once, so everything is built in release mode for the final toolchain. However, Foundation on linux is the one exception that builds twice in completely separate directories, with the second one thrown away after testing.

I see that the SwiftPM tests in the most recent run with this change are taking around just as long as the swift foundation tests were previously taking

SwiftPM is a little unusual in that it bootstraps a cut-down version of SwiftPM using CMake called swift-bootstrap, then uses that to build the final SwiftPM. Also, I now see that the final test run rebuilds a lot of SwiftPM a third time, even though it should be reusing most of the build objects from the second time, causing the test run to take so long.

Whatever inefficiencies currently exist in that SwiftPM build are not relevant to Foundation, which is doing a completely separate build in a new directory for its tests, unlike SwiftPM's second and third builds in the same build directory.

It seems reasonable to try to be consistent here

Consistent in what way? We don't want to be consistently slow. 😃 SwiftPM's problems with unnecessarily rebuilding the same files a third time don't apply here.

Just want to make sure we don't end up in a world where we've applied this change piecemeal to a handful of projects that doesn't leave a clear story as to when we should be testing in release mode and when we should be testing in debug mode.

So far, it's just one project, Foundation on linux, and a new --debug-foundation-tests flag in the preset would communicate that, if you're worried about making it clear.

It also might be worth looking at a breakdown of what is taking so long in this foundation build, I suspect a lot of it is likely coming from the release build of swift-syntax

Could be, unfortunately SwiftPM doesn't dump timestamps by default for each build step like ninja does.

There are two potential problems with this pull:

  1. The debug tests may behave differently - it sounds like you're not worried about that.
  2. We are not building everything in release mode as before, which may surprise devs running build-script - the suggested new --debug-foundation-tests flag added to the CI preset would communicate this clearly.

Let me know what you all think I should do next.

@jmschonfeld
Copy link
Contributor

@finagolfin ah I see ok thanks for the explanation - I hadn't realized that SwiftPM's test build was supposed to be re-using the artifacts from the normal build, I had thought it was also intentionally performing a separate test build (whether it was for @testable or otherwise).

So far, it's just one project, Foundation on linux

Does the same issue not exist on Windows? AFAIK Foundation also builds its test in release mode there as well. Re. the consistency, it sounds like consistency with SwiftPM isn't expected here but I think we should be consistent with how Foundation is built between platforms (unless there's a reason for it) so should Windows build/run tests for debug mode as well (or anything that uses build.ps1)?

The debug tests may behave differently - it sounds like you're not worried about that.

I'm still a bit worried that this will cause problems in the future, but I suspect it'll "Just Work" today so if the build time issues are very problematic this might be something worth doing despite that.

@finagolfin
Copy link
Member Author

Does the same issue not exist on Windows?

Not sure, this build-script is simply not used on Windows.

should Windows build/run tests for debug mode as well (or anything that uses build.ps1)?

I don't know what they do, perhaps @weliveindetail can fill us in.

I suspect it'll "Just Work" today so if the build time issues are very problematic this might be something worth doing despite that.

@shahmishal, you're in charge of the CI, let us know what you prefer, whenever you get some time again. I'll be using this patch for my local builds regardless.

@weliveindetail
Copy link
Member

should Windows build/run tests for debug mode as well (or anything that uses build.ps1)?

I don't know what they do, perhaps @weliveindetail can fill us in.

IIUC we do the same as Linux: build Foundation with CMake for distribution and with SwiftPM for testing in a separate directory. @jmschonfeld you implemented it in https://github.com/swiftlang/swift/pull/74594/files On first glance it seems simple to switch this step to debug mode, but I am not very familiar with it and I am sure about the impact.

Build times are generally long and reducing them seems reasonable. I can check with the team tomorrow and report back.

@weliveindetail
Copy link
Member

Apparently, we are doing incremental builds here in CI. Thus, this change won't have a drastic effect on build times and not a very a high prio.

@finagolfin
Copy link
Member Author

Thanks for the Windows info, Stefan, sounds like that CI is doing something completely different, so it's not relevant to this pull.

@finagolfin
Copy link
Member Author

Ping @shahmishal, this saves a significant 10-15% off of every full CI run if implemented, the equivalent of not having to build LLVM, whether in the simple way this pull does or by adding a flag first. Let me know what you think.

@shahmishal
Copy link
Member

Let’s take this change; however, we should create a new preset for the nightly toolchain to test release mode.

@finagolfin
Copy link
Member Author

OK, I will modify this pull to add a flag and that preset too.

@finagolfin
Copy link
Member Author

@swift-ci smoke test linux

@finagolfin
Copy link
Member Author

Alright, added a flag to control this and a new preset, as Mishal asked for. I only tested this to make sure the new build flag works, didn't run these new tests locally. I added the new flag to the most commonly used buildbot presets for linux that are run on the various CI, but probably not all of them.

I will make sure this passes CI tomorrow, in the meantime, need review on those choices. In particular, does the nightly toolchain build currently use the buildbot_linux preset like on this linux CI, @shahmishal? That is the preset I modified to add the single new preset that switches the foundation tests back to the slower release build: let me know if that's what you need or if the nightly currently uses another build preset.

@xavgru12
Copy link

How could possibly Debug mode be faster than Release?

@finagolfin
Copy link
Member Author

Simple, take a look at Alex's writeup on various ways to speed up the CI: this is 2. there, as these tests are dominated by build time not run time.

@finagolfin
Copy link
Member Author

@swift-ci smoke test

@finagolfin finagolfin changed the title Build the foundation tests in debug mode instead on linux [build-script] Add an option to build the Foundation tests in another mode Mar 20, 2025
… mode

Use it in the linux CI presets to set them to Debug mode and speed up the linux
CI, plus add a new preset which keeps building them in Release mode.
@finagolfin
Copy link
Member Author

@swift-ci smoke test

@finagolfin
Copy link
Member Author

OK, passed CI, @shahmishal, just let me know if I got the preset modifications right.

@finagolfin
Copy link
Member Author

@shahmishal, if you would confirm the preset changes, including the new nightly preset that does not do this new debug Foundation testing that you asked for, we can go ahead and get this in.

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.

5 participants