Add prepare for index experimental build argument #7574

dschaefer2 · 2024-05-17T15:23:32Z

This will be used by sourcekit-lsp to build the swiftmodule files it needs for indexing.

Adds experimental-prepare-for-indexing flag to swift build. Creates a llbuild manifest specific for the prepare build that skips generating object files and linking of those files and calls swiftc to only create the swiftmodule as quickly as it can.

Motivation:

To support background indexing in sourcekit-lsp, it will request a prepare for index build to build the swiftmodule files it needs to do indexing. This build should be minimal to ensure indexing is fast so it can respond to language server requests that require the index as soon as possible.

Modifications:

Add an experimental-prepare-for-indexing flag to the BuildOptions and pass it around to every that needs it.
Build a custom llbuild manifest so that only the commands necessary are added to the prepare build
Add a target property that also ensures tool builds required for the prepare build are performed as usual.
In SwiftTargetBuildDescription, pass compile options that put the swift compiler in prepare "mode".
Ensure object files and binaries are not generated when in prepare mode.

Result:

Implements prepare build mode without affecting regular build mode.

dschaefer2 · 2024-05-17T17:14:54Z

@swift-ci please test

dschaefer2 · 2024-05-17T17:57:33Z

@swift-ci please test

bnbarham · 2024-05-17T21:30:35Z

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

+                "-Xfrontend", "-experimental-skip-all-function-bodies",
+                "-Xfrontend", "-experimental-allow-module-with-compiler-errors",
+                "-Xfrontend", "-empty-abi-descriptor"
+            ]


We could also try out -experimental-lazy-typecheck and -experimental-skip-non-exportable-decls (sorry, meant to send these to you earlier). They're newer than the others, but should hopefully skip a reasonable amount of typechecking.

Will do. Currently just trying to get all the xplatform tests passing.

Wow, those two chopped my previous prepare time in half for sourcekit-lsp which happened in 30 seconds versus 95 for the debug build.

I had to enable-library-evolution for those to work, but that should be fine?

Hmmm, enabling library evolution would result in different diagnostics (eg. need to add @unknown case for switching over non-frozen enums). @tshortli is library evolution a hard requirement for these?

Wow, those two chopped my previous prepare time in half for sourcekit-lsp which happened in 30 seconds versus 95 for the debug build.

Just to be clear, 95s is the full debug build and a prepare used to be 60s without these and is now 30s?

Just to be clear, 95s is the full debug build and a prepare used to be 60s without these and is now 30s?

Correct. For sourcekit-lsp. I've been bouncing around. I need to do a more complete benchmark on a few different projects to get a better picture.

Although that restriction is about making sure the emitted module is suitable for use during compilation of downstream dependents

Yeah, sounds like we could probably gate the library evolution requirement check on -experimental-skip-all-function-bodies since we don't produce a swiftmodule that would be usable for compilation in that case anyway.

I've lifted the restriction so that -enable-library-evolution is no longer required when indexing:
apple/swift#73905
apple/swift#73905

Nice, thanks @tshortli!

I am still getting

<unknown>:0: warning: ignoring -experimental-skip-non-exportable-decls (requires -enable-library-evolution) <unknown>:0: warning: ignoring -experimental-lazy-typecheck (requires -enable-library-evolution)

in the latest 6.0 toolchain from May 26. Not sure this change got in after that build?

Would have been just after:
apple/swift#73906

bnbarham

Exciting!

Tests/BuildTests/PrepareForIndexTests.swift

dschaefer2 · 2024-05-17T22:02:33Z

@swift-ci please test

dschaefer2 · 2024-05-18T18:26:55Z

@swift-ci please test

dschaefer2 · 2024-05-18T18:37:49Z

LOL, oh yeah, because the test is actually failing. The object files are getting created on Linux. Need to find out why.

dschaefer2 · 2024-05-21T17:37:46Z

@swift-ci Please test

xedin · 2024-05-21T17:42:02Z

Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift

+            // Don't include the object nodes on prepare builds
+            cmdOutputs = [moduleNode]
+        } else {
+            cmdOutputs = objectNodes + [moduleNode]


Have you considered adjusting objects to produce [] if the target is in "preparation" mode?

Good point. I've been slowing moving these checks closer to the source of the values. I'll do one more pass with the help of your comments to clean that up. Thanks!

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

xedin · 2024-05-21T17:46:01Z

Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift

@@ -446,8 +455,15 @@ extension LLBuildManifestBuilder {
            case .swift(let target)?:
                inputs.append(file: target.moduleOutputPath)
            case .clang(let target)?:
-                for object in try target.objects {
-                    inputs.append(file: object)
+                if prepareForIndexing {


Could you elaborate on why this is necessary? This is handling a clang target, supposedly preparation doesn't apply here at all and these object files are produced by other clang targets?

In the normal case this adds a dependency on the object files from the clang modules to force a rebuild when they change. In preparation, there are no object files generated so this was a haphazard attempt to propagate the dependencies to their sources. This should actually be the headers. Let me give this one a bit more thought.

After giving this more thought, I need to keep the clang commands around so I can map the swiftmodule dependencies on C source files so that the modules get re-prepared when the C files change. I'll just replace them with phony commands so they don't actually get built.

After looking at the clang commands, I realized that the header dependencies are coming from the generated .d files. Since I'm not running the compiler they won't be there. So my original idea of adding the dependencies on the headers instead of the object files seems to be working.

Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift

Sources/Build/BuildManifest/LLBuildManifestBuilder.swift

Sources/CoreCommands/Options.swift

dschaefer2 · 2024-05-22T20:55:11Z

@swift-ci please test

bnbarham · 2024-05-24T05:45:27Z

@swift-ci please test macOS platform

bnbarham · 2024-05-24T05:45:38Z

@swift-ci please test Windows platform

This will be used by sourcekit-lsp to build the swiftmodule files it needs for indexing. Adds experimental-prepare-for-indexing flag to swift build. Creates a llbuild manifest specific for the prepare build that skips generating object files and linking of those files and calls swiftc to only create the swiftmodule as quickly as it can.

This was causing the builds to fail on other machines.

Simplify the tests to not assume names of the output and just check extensions.

At least until I can get a Linux dev env that actually works so I can run these locally.

Turns this off too when in prepare mode. Appears to be the default mode on Linux.

Moves the prepareForIndexing checks to more common points simplifing the checks. Also adds two more compile flags which makes the preparation much faster.

Makes it clearer there are no object files involved with the swift build for preparation.

dschaefer2 · 2024-05-29T14:03:40Z

@swift-ci please test

dschaefer2 · 2024-05-29T14:03:53Z

@swift-ci please test Windows platform

We're already testing for swiftmodules and object files in the other asserts.

dschaefer2 · 2024-05-29T15:18:34Z

@swift-ci please test

dschaefer2 · 2024-05-29T16:10:00Z

@swift-ci please test Windows platform

MaxDesiatov · 2024-05-29T16:15:59Z

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

@@ -582,6 +582,17 @@ public final class SwiftTargetBuildDescription {
            args += ["-emit-module-interface-path", self.parseableModuleInterfaceOutputPath.pathString]
        }

+        if self.defaultBuildParameters.prepareForIndexing {
+            args += [
+                "-Xfrontend", "-enable-library-evolution",


Is -enable-library-evolution needed here now?

Was waiting for an update toolchain. There's one there now. Should be ready to remove.

Would you mind updating the diff accordingly before merging?

Will do. That will be the last piece. Once I can make sure it works 😀

If this is the only thing blocking the PR from being merged, I’d prefer to get the PR merged with -enable-library-evolution still set because it could unblock me to test preparation in sourcekit-lsp. I know that I’ll get spurious warnings because of library evolution but I can ignore those for now.

Yeah. As mentioned above, I'm not even sure if the change that was made even fixes it. I have one more fix Ben B mentioned about hiding the flag and then I'll kick off the tests.

Tests/BuildTests/PrepareForIndexTests.swift

dschaefer2 · 2024-05-29T20:14:55Z

@ahoppen really wants me to checkout multi target prepare. I will look at that next.

bnbarham · 2024-05-30T00:50:32Z

@ahoppen really wants me to checkout multi target prepare. I will look at that next.

We can merge this without that. That's also something we should all discuss a bit, since it would be weird to have that for only preparation.

dschaefer2 · 2024-05-30T00:52:27Z

@ahoppen really wants me to checkout multi target prepare. I will look at that next.

We can merge this without that. That's also something we should all discuss a bit, since it would be weird to have that for only preparation.

Agreed. I'm not even sure I can make it work cleanly with the current architecture. I'd rather not rush it.

…o the prepare command The main purpose for now is that this makes it easier for me to live on background indexing combined with apple/swift-package-manager#7574.

bnbarham · 2024-05-31T06:10:03Z

Sources/CoreCommands/Options.swift

@@ -437,6 +437,9 @@ public struct BuildOptions: ParsableArguments {
    @Flag(help: "Enable or disable indexing-while-building feature")
    public var indexStoreMode: StoreMode = .autoIndexStore

+    @Flag(name: .customLong("experimental-prepare-for-indexing"))


Might be a good idea to have this hidden (noticed in apple/sourcekit-lsp#1373)

Agreed. This option isn't really useful to humans :).

…o the prepare command The main purpose for now is that this makes it easier for me to live on background indexing combined with apple/swift-package-manager#7574.

dschaefer2 · 2024-06-01T21:15:16Z

@swift-ci please test

dschaefer2 · 2024-06-01T21:28:21Z

@swift-ci please test Windows platform

dschaefer2 requested review from bnbarham, MaxDesiatov, jakepetroules and francescomikulis as code owners May 17, 2024 15:23

bnbarham requested a review from xedin May 17, 2024 21:24

bnbarham reviewed May 17, 2024

View reviewed changes

Tests/BuildTests/PrepareForIndexTests.swift Outdated Show resolved Hide resolved

Tests/BuildTests/PrepareForIndexTests.swift Outdated Show resolved Hide resolved

MaxDesiatov changed the title ~~Adds prepare for index experimental build argument.~~ Add prepare for index experimental build argument May 19, 2024

xedin reviewed May 21, 2024

View reviewed changes

apple deleted a comment from domciiiiix May 27, 2024

dschaefer2 added 7 commits May 28, 2024 16:53

Fix prepare for index tests to not include triple.

b0d7a86

This was causing the builds to fail on other machines.

Account for Linux producing different outputs.

0740be5

Simplify the tests to not assume names of the output and just check extensions.

Add more output to test failure on Linux.

348c08d

At least until I can get a Linux dev env that actually works so I can run these locally.

Turn off creating the modulewrap object files.

26996d5

Turns this off too when in prepare mode. Appears to be the default mode on Linux.

Simplify the check for prepareForIndexing, new swiftc flags.

abd1224

Moves the prepareForIndexing checks to more common points simplifing the checks. Also adds two more compile flags which makes the preparation much faster.

Clean up handling of objectNodes in Swift compile command.

b513d5a

Makes it clearer there are no object files involved with the swift build for preparation.

dschaefer2 force-pushed the prepare branch from bb9ba1e to b513d5a Compare May 29, 2024 14:00

Merge remote-tracking branch 'origin/main' into prepare

c821697

Reduce dependency on command names which now have target triples.

1aa46c0

We're already testing for swiftmodules and object files in the other asserts.

MaxDesiatov reviewed May 29, 2024

View reviewed changes

Tests/BuildTests/PrepareForIndexTests.swift Outdated Show resolved Hide resolved

Fix header on PrepareForIndexTests

377bb69

ahoppen mentioned this pull request May 31, 2024

When SwiftPM supports --experimental-prepare-for-indexing pass it to the prepare command apple/sourcekit-lsp#1373

Merged

bnbarham reviewed May 31, 2024

View reviewed changes

dschaefer2 added 2 commits June 1, 2024 17:02

Hide the prepare for indexing flag from the help text.

c5e892a

Merge remote-tracking branch 'origin' into prepare

64487b3

bnbarham approved these changes Jun 1, 2024

View reviewed changes

dschaefer2 merged commit 7bd34cc into apple:main Jun 2, 2024
5 checks passed

Add prepare for index experimental build argument #7574

Add prepare for index experimental build argument #7574

Conversation

dschaefer2 commented May 17, 2024

Motivation:

Modifications:

Result:

dschaefer2 commented May 17, 2024

dschaefer2 commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnbarham May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnbarham May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnbarham left a comment

Choose a reason for hiding this comment

dschaefer2 commented May 17, 2024

dschaefer2 commented May 18, 2024

dschaefer2 commented May 18, 2024

dschaefer2 commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschaefer2 commented May 22, 2024

bnbarham commented May 24, 2024

bnbarham commented May 24, 2024

dschaefer2 commented May 29, 2024

dschaefer2 commented May 29, 2024

dschaefer2 commented May 29, 2024

dschaefer2 commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschaefer2 May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschaefer2 commented May 29, 2024

bnbarham commented May 30, 2024

dschaefer2 commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschaefer2 commented Jun 1, 2024

dschaefer2 commented Jun 1, 2024

bnbarham May 22, 2024 •

edited

bnbarham May 22, 2024 •

edited

dschaefer2 May 30, 2024 •

edited