Skip to content

Incremental: Don't rebuild everything when only emitting the module #2102

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.

Sign up for GitHub

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
benb wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −20
Open

Incremental: Don't rebuild everything when only emitting the module #2102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions Sources/SwiftDriver/Jobs/EmitModuleJob.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,11 @@ extension Driver {
// are risking collisions in output filenames.
//
// In cases where other compile jobs exist, they will produce dependency outputs already.
// There are currently no cases where this is the only job because even an `-emit-module`
// driver invocation currently still involves partial compilation jobs.
// When partial compilation jobs are removed for the `compilerOutputType == .swiftModule`
// case, this will need to be changed here.
// When compilerOutputType == .swiftModule, there are no per-file compile jobs (they are
// skipped since emitModuleSeparately handles the module), so the emit-module job must
// produce .dependencies output itself.
//
if emitModuleSeparately {
if emitModuleSeparately && compilerOutputType != .swiftModule {
return
}
Comment on lines +60 to 62
Copy link
Member

@compnerd compnerd Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this - how is this different from L45-47?

Copy link
Contributor

@artemcm artemcm Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is meant to relax the check on this line, but it does make it fully redundant in the process.

Copy link
Contributor Author

@benb benb Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ach, I forgot about this weirdness. Yes, it's redundant this way. Kicking myself.

if let dependenciesFilePath = dependenciesFilePath {
Expand Down
6 changes: 4 additions & 2 deletions Sources/SwiftDriver/Jobs/Planning.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,8 +434,10 @@ extension Driver {

assert(input.type.isPartOfSwiftCompilation)
// We can skip the compile jobs if all we want is a module when it's
// built separately.
// built separately, unless we need per-file outputs like const values
// that only compile jobs can produce.
let canSkipIfOnlyModule = compilerOutputType == .swiftModule && emitModuleSeparately
&& !parsedOptions.hasArgument(.emitConstValues)
Copy link
Member

@compnerd compnerd Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some classification that we can use? "like const values" ... are there other features would also require compile jobs?

Copy link
Contributor

@artemcm artemcm Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to this, this doesn't seem like a general approach, there are many other kinds of outputs which may be requested on a driver invocation which are only produced by object compilation (not emit-module) tasks.

I think we should trust the compilerOutputType and for the case you are trying to catch here perhaps we instead have the driver diagnose when the invocation specifies a module output compiler "mode" but also specifies requiring some outputs which require compilation tasks which produce object files.

We select the compiler output type here:

swift-driver/Sources/SwiftDriver/Driver/Driver.swift

Line 2442 in 4e94366

private static func determinePrimaryOutputs(

Based on which compilation "mode" flag is used. Admittedly this part of the driver logic seems quite messy because -emit-module seems to be considered a mode only if no other .modes flag was specified.

But, if we are in a compilerOutputType == .swiftModule scenario, -emit-module doesn't mean "among other things, emit a module", but rather "this swiftc invocation's intent is to emit a module". It's reasonable that some outputs will not be compatible with this mode.

An alternative would be to extend determinePrimaryOutputs to select a different compilerOutputType when things like .emitConstValues are specified.

Copy link
Contributor Author

@benb benb Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me paraphrase you, and you can tell me if I have this right:

Given the current implementation of determinePrimaryOutputs, if we have compilerOutputType == .swiftModule then we must be in an de facto emit module 'mode' (albeit this is not an actual Option.Group.Modes mode like .emitExecutable).

Therefore one way of looking at is: When only specifying emit-module you are not really expecting to receive other outputs here, so we don't need to worry about them here.

Option two would be to catch other outputs (supplementaryOutputs?) in determinePrimaryOutputs and change the compilerOutputType to reflect this.

My opinion here is that it seems weird to make a change so that Swiftmodule is not be the 'primary output' in these cases - it really is the primary output! So I would be minded to 'trust' the compilerOutputType and remove this change.

Does this make sense?

try createAndAddCompileJob(primaryInput: input,
emitModuleTrace: emitModuleTrace,
canSkipIfOnlyModule: canSkipIfOnlyModule,
Expand Down Expand Up @@ -478,7 +480,7 @@ extension Driver {
) throws {
// We can skip the compile jobs if all we want is a module when it's
// built separately.
if parsedOptions.hasArgument(.driverExplicitModuleBuild), canSkipIfOnlyModule { return }
if canSkipIfOnlyModule { return }
Copy link
Contributor

@artemcm artemcm Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change I've wanted to make for a while since it is rather superfluous and leads to an odd difference between implicit and explicit builds.

While I think we should do it, I am having a hard time remembering if this is still an assumption some part of swift-build relies on. @owenv do you recall?

Copy link
Contributor

@owenv owenv Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might break some assumptions but now that we've open sourced swift-build it's a lot easier to test. I'll try to take a closer look soon but I might not get to it until next week

// If we are in the batch mode, the constructed jobs here will be batched
// later. There is no need to produce cache key for the job.
let compile = try compileJob(primaryInputs: [primaryInput],
Expand Down
16 changes: 8 additions & 8 deletions Tests/SwiftDriverTests/IncrementalCompilationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,9 @@ extension IncrementalCompilationTests {
XCTAssertEqual(outputs.first!.file.absolutePath, expected)
}

// Null planning should not return an empty compile job for compatibility reason.
// `swift-build` wraps the jobs returned by swift-driver in `Executor` so returning an empty list of compile job will break build system.
// When compilerOutputType == .swiftModule and emitModuleSeparately is true,
// per-file compile jobs are skipped since the emit-module job handles
// everything. Verify that the null build still returns the emit-module job.
func testNullPlanningCompatibility() throws {
guard let sdkArgumentsForTesting = try Driver.sdkArgumentsForTesting() else {
throw XCTSkip("Cannot perform this test on this host")
Expand All @@ -232,16 +233,15 @@ extension IncrementalCompilationTests {
var driver = try Driver(args: commonArgs + extraArguments + sdkArgumentsForTesting)
let initialJobs = try driver.planBuild()
XCTAssertTrue(initialJobs.contains { $0.kind == .emitModule})
// Module-only builds should not have compile jobs since emitModuleSeparately
// handles the module output.
XCTAssertTrue(initialJobs.filter { $0.kind == .compile }.isEmpty)
try driver.run(jobs: initialJobs)

// Plan the build again without touching any file. This should be a null build but for compatibility reason,
// planBuild() should return all the jobs and supported build system will query incremental state for the actual
// jobs need to be executed.
// Plan the build again without touching any file. This should be a null build.
let replanJobs = try driver.planBuild()
XCTAssertFalse(
replanJobs.filter { $0.kind == .compile }.isEmpty,
"more than one compile job needs to be planned")
XCTAssertTrue(replanJobs.contains { $0.kind == .emitModule})
XCTAssertTrue(replanJobs.filter { $0.kind == .compile }.isEmpty)
}
}

Expand Down
10 changes: 5 additions & 5 deletions Tests/SwiftDriverTests/SwiftDriverTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2026,7 +2026,7 @@ final class SwiftDriverTests: XCTestCase {
"-output-file-map", outputFileMap.description])
let plannedJobs = try driver.planBuild().removingAutolinkExtractJobs()

XCTAssertEqual(plannedJobs.count, 3)
XCTAssertEqual(plannedJobs.count, 1)
XCTAssertEqual(plannedJobs[0].kind, .emitModule)
try XCTAssertJobInvocationMatches(plannedJobs[0], .flag("-serialize-diagnostics-path"), .path(.absolute(.init(validating: "/build/Foo-test.dia"))))
}
Expand All @@ -2051,7 +2051,7 @@ final class SwiftDriverTests: XCTestCase {
XCTAssertTrue(driver.diagnosticEngine.diagnostics.isEmpty)

// Test the output path is correct for GeneratePCH job.
XCTAssertEqual(plannedJobs.count, 4)
XCTAssertEqual(plannedJobs.count, 2)
XCTAssertEqual(plannedJobs[0].kind, .generatePCH)
try XCTAssertJobInvocationMatches(plannedJobs[0], .flag("-o"), .path(.absolute(.init(validating: "/build/Foo-bridging-header.pch"))))

Expand Down Expand Up @@ -3911,7 +3911,7 @@ final class SwiftDriverTests: XCTestCase {
var driver = try Driver(args: ["swiftc", "-module-name=ThisModule", "main.swift", "multi-threaded.swift", "-emit-module", "-o", "test.swiftmodule", "-experimental-emit-module-separately"])
let plannedJobs = try driver.planBuild().removingAutolinkExtractJobs()

XCTAssertEqual(plannedJobs.count, 3)
XCTAssertEqual(plannedJobs.count, 1)

XCTAssertEqual(plannedJobs[0].kind, .emitModule)
XCTAssertJobInvocationMatches(plannedJobs[0], .flag("-emit-abi-descriptor-path"))
Expand Down Expand Up @@ -4636,8 +4636,8 @@ final class SwiftDriverTests: XCTestCase {
// -experimental-emit-module-separately.
var driver = try Driver(args: ["swiftc", "foo.swift", "bar.swift", "-module-name", "Test", "-emit-module-path", rebase("Test.swiftmodule", at: root), "-experimental-emit-module-separately"])
let plannedJobs = try driver.planBuild().removingAutolinkExtractJobs()
XCTAssertEqual(plannedJobs.count, 3)
XCTAssertEqual(Set(plannedJobs.map { $0.kind }), Set([.emitModule, .compile]))
XCTAssertEqual(plannedJobs.count, 1)
XCTAssertEqual(Set(plannedJobs.map { $0.kind }), Set([.emitModule]))
Copy link
Member

@compnerd compnerd Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simplify this?

Suggested change
XCTAssertEqual(Set(plannedJobs.map { $0.kind }), Set([.emitModule]))
XCTAssertEqual(plannedJobs[0].kind, .emitModule)

XCTAssertTrue(plannedJobs[0].tool.name.contains("swift"))
XCTAssertEqual(plannedJobs[0].outputs.count, driver.targetTriple.isDarwin ? 4 : 3)
XCTAssertEqual(plannedJobs[0].outputs[0].file, .absolute(try .init(validating: rebase("Test.swiftmodule", at: root))))
Expand Down