Incremental: Don't rebuild everything when only emitting the module#2102
Incremental: Don't rebuild everything when only emitting the module#2102benb wants to merge 1 commit intoswiftlang:mainfrom
Conversation
benb
requested review from
DougGregor,
artemcm,
cachemeifyoucan,
compnerd and
owenv
as code owners
| 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])) |
There was a problem hiding this comment.
Why not simplify this?
| XCTAssertEqual(Set(plannedJobs.map { $0.kind }), Set([.emitModule])) | |
| XCTAssertEqual(plannedJobs[0].kind, .emitModule) |
| // 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) |
There was a problem hiding this comment.
Is there some classification that we can use? "like const values" ... are there other features would also require compile jobs?
There was a problem hiding this comment.
+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:
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.
There was a problem hiding this comment.
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?
| if emitModuleSeparately && compilerOutputType != .swiftModule { | ||
| return | ||
| } |
There was a problem hiding this comment.
I don't understand this - how is this different from L45-47?
There was a problem hiding this comment.
I guess this is meant to relax the check on this line, but it does make it fully redundant in the process.
There was a problem hiding this comment.
Ach, I forgot about this weirdness. Yes, it's redundant this way. Kicking myself.
| if emitModuleSeparately && compilerOutputType != .swiftModule { | ||
| return | ||
| } |
There was a problem hiding this comment.
I guess this is meant to relax the check on this line, but it does make it fully redundant in the process.
| // 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) |
There was a problem hiding this comment.
+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:
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.
| // 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 } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
4 participants
Currently, if we want to emit a swiftmodule / header only in incremental mode, swift-driver will schedule the work for rebuilding the object outputs regardless (although it won't actually produce that output).
This PR makes these changes:
.driverExplicitModuleBuildoption (i.e.explicitSwiftModuleMap), and as far as I can tell checking for explicit module builds is overly conservative here anyway.I might be barking up the wrong tree here, if so I'd appreciate any guidance on how else we can resolve this.