Read and Write The Driver's Dependency Graph For Cross-Module Builds #467
Conversation
|
@swift-ci test |
| let diagnosticEngine = driver.diagnosticEngine | ||
| // If we cannot compute this path, it means the build record path was not | ||
| // provided to the driver, which our caller detects for us. | ||
| let absPath = driver.driverDependencyGraphPath! |
There was a problem hiding this comment.
In the past, we've run into trouble with disallowing relative paths.
| /// | ||
| /// FIXME: This is a little ridiculous. We could probably just replace the | ||
| /// build record outright with a serialized format. | ||
| var driverDependencyGraphPath: AbsolutePath? { |
There was a problem hiding this comment.
I suspect that this has to be a VirtualPath for the tests to run.
| let compileGroups = | ||
| Dictionary(uniqueKeysWithValues: | ||
| jobsInPhases.compileGroups.map { ($0.primaryInput, $0) }) |
There was a problem hiding this comment.
Would be nice to move this computation to just before using compileGroups
| // We failed to read the components we need for the incremental build - | ||
| // just schedule every file to build! |
There was a problem hiding this comment.
Is there some reason not to try a build reading the swiftDeps?
This raises another question: as it is, the code has two paths:
- status quo - don't try to read the saved graph, don't try to exploit incrementalExternalDeps
- cross module - do both
In other words, maybe there are four logical possibilities--I haven't thought about if it's best to collapse it to these two.
| let mandatoryJobsInOrder = try | ||
| jobsInPhases.beforeCompiles + | ||
| driver.formBatchedJobs( | ||
| mandatoryCompileGroupsInOrder.flatMap {$0.allJobs()}, | ||
| showJobLifecycle: driver.showJobLifecycle) |
There was a problem hiding this comment.
Because this computation is submerged in the other case, I'm not sure about seeing it here; need to think on it. Oh, I see, this is in an error-recovery path.
| private static func computeInitialStateForStatusQuoBuild( | ||
| _ options: Options, | ||
| _ jobsInPhases: JobsInPhases, | ||
| _ outputFileMap: OutputFileMap, | ||
| _ buildRecordInfo: BuildRecordInfo, | ||
| _ driver: inout Driver, | ||
| _ reporter: Reporter? | ||
| ) throws -> InitialState? { | ||
| precondition(!options.contains(.enableCrossModuleIncrementalBuild)) | ||
| let diagnosticEngine = driver.diagnosticEngine | ||
|
|
||
| // FIXME: This should work without an output file map. We should have | ||
| // another way to specify a build record and where to put intermediates. | ||
| guard let outOfDateBuildRecord = buildRecordInfo.populateOutOfDateBuildRecord( | ||
| inputFiles: driver.inputFiles, reporter: reporter) | ||
| else { | ||
| return nil | ||
| } | ||
|
|
||
| let missingInputs = Set(outOfDateBuildRecord.inputInfos.keys).subtracting(driver.inputFiles.map {$0.file}) | ||
| guard missingInputs.isEmpty else { | ||
| if let reporter = reporter { | ||
| reporter.report( | ||
| "Incremental compilation has been disabled, " + | ||
| " because the following inputs were used in the previous compilation but not in this one: " | ||
| + missingInputs.map {$0.basename} .joined(separator: ", ")) | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
As I wrote above, I'm not sure about this factoring. I'd be tempted to:
Read the serialized Mod dep graph in any case,
and go from there. But maybe that's too much instability??
| let outOfDateBuildRecord = buildRecordInfo.populateOutOfDateBuildRecord( | ||
| inputFiles: driver.inputFiles, reporter: reporter) |
There was a problem hiding this comment.
This same computation and the aborting incremental building appears in both the statusQuo and crossModule code. Feels like it should be in a caller and passed in if non-null.
| // MARK: - Serialization | ||
|
|
||
| extension IncrementalCompilationState { | ||
| @_spi(Testing) public func writeDependencyGraph() { |
There was a problem hiding this comment.
In other places the _spi is commented out. We went through some kerfuffle wrt testable vs _spi. I'm not sure what the current state is.
| // If the cross-module build is not enabled, the status quo dictates we | ||
| // not emit this file. | ||
| guard self.options.contains(.enableCrossModuleIncrementalBuild) else { | ||
| return | ||
| } |
There was a problem hiding this comment.
I'd be tempted to do so anyway, speeding up the non-cross-module case.
| /// Builds an empty `ModuleDependencyGraph` for a compilation with the | ||
| /// given input files. | ||
| static func buildEmptyGraph<Inputs: Sequence>( | ||
| for inputFiles: Inputs, | ||
| diagnosticEngine: DiagnosticsEngine, | ||
| outputFileMap: OutputFileMap, | ||
| options: IncrementalCompilationState.Options, | ||
| reporter: IncrementalCompilationState.Reporter? | ||
| ) -> ModuleDependencyGraph | ||
| where Inputs.Element == TypedVirtualPath | ||
| { | ||
| let emptyGraph = ModuleDependencyGraph(diagnosticEngine: diagnosticEngine, | ||
| reporter: reporter, | ||
| options: options) | ||
|
|
||
| for input in inputFiles { | ||
| guard let swiftDepsFile = outputFileMap.existingOutput(inputFile: input.file, | ||
| outputType: .swiftDeps) else { | ||
| continue | ||
| } | ||
|
|
||
| assert(swiftDepsFile.extension == FileType.swiftDeps.rawValue) | ||
| let typedSwiftDepsFile = TypedVirtualPath(file: swiftDepsFile, type: .swiftDeps) | ||
| let dependencySource = ModuleDependencyGraph.DependencySource(typedSwiftDepsFile) | ||
| emptyGraph.inputDependencySourceMap[input] = dependencySource | ||
| } | ||
| return emptyGraph | ||
| } |
There was a problem hiding this comment.
I don't think we ought to need to do this; just fall back to the buildInitialGraph path. Am I missing something?
CodaFi
force-pushed
the
reader-writer-locks
branch
2 times, most recently
from
010901a to
1ecfe52
Compare
The refactoring of the incremental compilation state class is in three pieces: 1) Formalize the type of the InitialState computed by the initializer 2) Internalize computeModuleDependencyGraph and instead make a unified computeInitialState entrypoint (we'll need this for testing later) 3) Divide the status quo and cross-module initial state computations. There is no change to the status quo outside of the breakup of Driver.getBuildInfo The cross-module path is slightly different than the status quo. For one, the incremental build need not fail entirely when either the build record or dependency graph cannot be read. In fact, it is a requirement that the incremental build proceed but schedule every single input file to rebuild, otherwise we won't have a module dependency graph to serialize! As an aside, we ought to consider unifying the build record with the module dependency graph state we're writing here. They're read and written in lock-step when cross-module builds are turned on.
CodaFi
force-pushed
the
reader-writer-locks
branch
from
1ecfe52 to
93d710a
Compare
|
@swift-ci test |
The refactoring of the incremental compilation state class is in three pieces:
The cross-module path is slightly different than the status quo. For one, the incremental build need not fail entirely when either the build record or dependency graph cannot be read. In fact, it is a requirement that the incremental build proceed but schedule every single input file to rebuild, otherwise we won't have a module dependency graph to serialize!
As an aside, we ought to consider unifying the build record with the module dependency graph state we're writing here. They're read and written in lock-step when cross-module builds are turned on.