Improve .build caching for faster builds

[Sencudra]

Saw the forum discussion about slow build time. Spent some time to research this as no one is planning to do this for now.

There are two major changes that showed significant improvement compared to other approaches I’ve tried:

1. Improved caching logic by warming up all the required dependencies. In the current version of the test runner, this is only done for the Numerics package.
2. Changed the build directory from a Docker-mounted volume to a working directory inside the container. For some reason at least for me it significantly improve build time.

The testing was performed on my local machine using bin/benchmark-in-docker.sh and an improved version of single-test-that-passes. That changes are not in this PR but, locally I've added various imports such as Foundation and Numerics. The results are as follows:

• Current test runner version: 14.277 s ± 0.549 s
• Improved cache: 8.787 s ± 0.726 s
• Improved cache + changed build directory: 6.045 s ± 0.341 s

Improving cache

Now I want to briefly guide you through my findings.. To understand what’s happening during the build, you can pass the verbose flag to any Swift command, for example: swift build -vv. There’s a lot going on under the hood, with the majority of the work done by swiftc and swift-frontend calls. It turns out that instead of building just two modules—the target under test and the test target—it actually builds four:

• Package
• PackageTests
• PackageDiscoveredTests
• PackagePackageTests (whatever that was)

It looks like PackageDiscoveredTests is related to test auto-discovery. So, maybe that was not the best desicion to get rid of manual test description. But anyway...

This is what happens during the swift-frontend invocation:

swift test -Xswiftc -stats-output-dir -Xswiftc ./packet.stats -Xswiftc -trace-stats-events -Xswiftc -driver-time-compilation

---User Time---  --System Time--  --User+System--  ---Wall Time--- --- Name ---
0.0100 ( 2.2%)  0.0059 ( 9.5%)  0.0159 ( 3.1%)  1.3145 ( 41.9%) parse-and-resolve-imports
0.0099 ( 2.2%)  0.0059 ( 9.5%)  0.0157 ( 3.1%)  1.3143 ( 41.9%) Import resolution
0.1387 ( 30.8%) 0.0065 ( 10.5%) 0.1452 ( 28.4%) 0.1450 ( 4.6%) build-rewrite-system
0.0448 ( 10.0%) 0.0103 ( 16.5%) 0.0551 ( 10.8%) 0.0551 ( 1.8%) perform-sema
0.0447 ( 9.9%)  0.0103 ( 16.5%) 0.0550 ( 10.7%) 0.0550 ( 1.8%) Type checking and Semantic analysis
0.0446 ( 9.9%)  0.0041 ( 6.7%)  0.0487 ( 9.5%)  0.0487 ( 1.6%) typecheck-stmt
0.0378 ( 8.4%)  0.0044 ( 7.0%)  0.0422 ( 8.2%)  0.0422 ( 1.3%) typecheck-expr
0.0249 ( 5.5%)  0.0099 ( 15.9%) 0.0348 ( 6.8%)  0.0348 ( 1.1%) typecheck-decl
0.0000 ( 0.0%)  0.0040 ( 6.4%)  0.0040 ( 0.8%)  0.0310 ( 1.0%) load-stdlib
0.0285 ( 6.3%)  0.0009 ( 1.4%)  0.0294 ( 5.7%)  0.0295 ( 0.9%) import-clang-decl
0.0172 ( 3.8%)  0.0000 ( 0.0%)  0.0172 ( 3.4%)  0.0172 ( 0.5%) SILGen
0.0147 ( 3.3%)  0.0000 ( 0.0%)  0.0147 ( 2.9%)  0.0147 ( 0.5%) SILGen-function
0.0138 ( 3.1%)  0.0001 ( 0.1%)  0.0139 ( 2.7%)  0.0139 ( 0.4%) precheck-target
0.0086 ( 1.9%)  0.0000 ( 0.0%)  0.0086 ( 1.7%)  0.0086 ( 0.3%) SIL optimization
0.0065 ( 1.4%)  0.0000 ( 0.0%)  0.0065 ( 1.3%)  0.0065 ( 0.2%) IRGen
0.0049 ( 1.1%)  0.0000 ( 0.0%)  0.0049 ( 1.0%)  0.0049 ( 0.2%) typecheck-for-each
0.0001 ( 0.0%)  0.0000 ( 0.0%)  0.0001 ( 0.0%)  0.0001 ( 0.0%) source-file-populate-cache
0.0001 ( 0.0%)  0.0000 ( 0.0%)  0.0001 ( 0.0%)  0.0001 ( 0.0%) get-conformance-access-path
0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%) SIL verification, pre-optimization
0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%) populate-source-file-class-member-cache
0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%) SIL verification, post-optimization
0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%) module-populate-cache
0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%) AST verification
0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%)  0.0000 ( 0.0%) perform-whole-module-type-checking
0.4498 (100.0%) 0.0622 (100.0%) 0.5120 (100.0%) 3.1361 (100.0%) Total

Almost 3 seconds is wasted on parsing, resolving imports, and import resolution. It turns out that during this time, swift-frontend builds all the internal dependencies (clang and swift modules) required for the files for each package. Then can be seen in ModuleCache directory.

So the idea of this one is two create a package called WarmUp, place all the imports. Then compile and store .build directory into docker image. It's already done for Numerics package. Everything else is added in this PR.

Changing build directory

It turns out building inside docker image is more efficient then working with mounted directory.

Chanages in .sh scripts

Current scripts are used in two environments: docker image and local. This two envs differ. Docker environment containes one solution (exercise) in it's root directory with .build, Package.resolved and bin folders (according to Dockerfile). For local runs everything is performed in an exercise directory. Scripts are adopted to support both ways of running. I've also added run-tests.sh to run all tests locally.

[github-actions]

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.

---

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

[github-actions]

Hello 👋 Thanks for your PR.

This repo does not currently have dedicated maintainers. Our guardians team will attempt to review and merge your PR, but it will likely take longer for your PR to be reviewed.

If you enjoy contributing to Exercism and have a track-record of doing so successfully, you might like to become an Exercism maintainer for this track.

Please feel free to ask any questions, or chat to us about anything to do with this PR or the reviewing process on the Exercism forum.

(cc @exercism/guardians)

[Sencudra]

Not intuitive, but it will actually throw an error if path to .pcm will be different.

[Sencudra]

Was a warning for deprecated style of env vars

[SleeplessByte]

Is the NAME env variable (set to empty) no longer needed?

[Sencudra]

Actually looks redundant, Cause the actual ENV is set in run.sh. Will try removing it.

[meatball133]

I get the same error as the ci, could you look into fixing the ci. Otherwise, am I for the idea of more caching.

[Sencudra]

I get the same error as the ci, could you look into fixing the ci. Otherwise, am I for the idea of more caching.

So the problems was due to changes of build directory for test package from mounted directory to interal working directory. To fix I've changed some scripts. The major change is that instead of running all the tests inside one docker image, now for every test a new clean image is used. Also, I've added run-tests.sh for local runs. Now at least for me all scripts run okay.

Scripts are becoming hard to maintain, maybe in the future it's a good idea to get rid of non-docker scripts.

@meatball133, when will it be possible, please launch the PR checks

[SleeplessByte]

@Sencudra I have approved running the CI. You can ping @exercism/guardians should you need another approved run.

[SleeplessByte]

Overall looks fine. @meatball133 ping when you want a guardian approval to merge.

[SleeplessByte]

You may want to combine these to reduce the number of layers.

[SleeplessByte]

You may want to combine these to reduce the number of layers.

[Sencudra]

Not sure it's possible here. Each copy operation has it's own destination: TestRunner and Pacage.resolved are files, .build is a directory.

[SleeplessByte]

Is the NAME env variable (set to empty) no longer needed?

[Sencudra]

@exercism/guardians please, approve the workflow

[meatball133]

I have looked through the changes. Sorry for taking some time, over the Summer I have had kind of 1-2 days per week where I can work on GitHub etc, and the rest goes to other stuff.

What I am a bit conflicted about is the fact that more or less all bin files (I find it quite neat when all of these files (except run.sh) are the same across tracks). But this pr, to its core just adds more core packages to be cached. I don't know if copying these files will be too big of a bottleneck, but otherwise I don't really see the need for such a heavy restructure, even if yes it would save on execution time.

I am not saying that I am against the changes but I will think about it to tomorrow what I think about it, I will also do some performance testing. But everything else looks good and it seems to be passing the tests.

[Sencudra]

@meatball133 Take as much time as you need.

A few words about the restructuring. Before this PR, the folder with the exercise was supposed to be mounted into the Docker image. That part hasn’t changed — except now, instead of building in the mounted folder, it copies all the contents (the exercise package) into the Docker image and builds it there, inside. Tests showed an improvement — at least on my machine.

So, here is the question. The old run-test-docker.sh ran all the tests in the same Docker image, so to work it now it's necessary to have a script that removes artifacts between test runs. Not only to make it work properly, but also to ensure that the benchmark is accurate and independent between runs. So, between deleting artifacts manually and running a clean Docker image, I chose the latter, since deletion didn't look trivial. You're supposed to delete all the artifacts while preserving things like Package.resolved, bin/, etc. Not knowing that all the runners follow the same file structure, I went with the approach to just use a clean docker for each test. That's why scripts have changed.

I don't really see the need for such a heavy restructure, even if yes it would save on execution time.

That would be a valid point — if the runner didn’t hit Docker timeouts. Overall, I believe the need to preserve the file structure pattern in the repository should not outweigh the effectiveness and simplicity of the code.

[meatball133]

Sorry for taking some time before replying. I have decided to go ahead with the changes even though they change the structure.