From d56cadf2568cfe614deb7865ed91a9642d622395 Mon Sep 17 00:00:00 2001 From: David Ungar Date: Mon, 5 Jul 2021 23:45:43 -0700 Subject: [PATCH 01/16] Add remark to show when sources are read --- .../ModuleDependencyGraphParts/DependencySource.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift index e7f60b9b0..f1e2f9b6e 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift @@ -55,6 +55,7 @@ extension DependencySource { ) -> SourceFileDependencyGraph? { guard let fileToRead = fileToRead(info: info) else {return nil} do { + info.reporter?.report("Reading deps from \(description)") return try SourceFileDependencyGraph.read(from: fileToRead, on: info.fileSystem) } catch { From f808674e37bfa0a0d4e7a4d68954583789890dfb Mon Sep 17 00:00:00 2001 From: David Ungar Date: Tue, 6 Jul 2021 11:57:44 -0700 Subject: [PATCH 02/16] Redo external integration --- .../DependencyKey.swift | 4 +- .../ModuleDependencyGraph.swift | 264 +++++++++++------- .../Integrator.swift | 3 +- .../ModuleDependencyGraphTests.swift | 6 +- 4 files changed, 174 insertions(+), 103 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/DependencyKey.swift b/Sources/SwiftDriver/IncrementalCompilation/DependencyKey.swift index 8fe1092d2..386e9520c 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/DependencyKey.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/DependencyKey.swift @@ -348,7 +348,7 @@ extension DependencyKey.Designator: Comparable {} // MARK: - InvalidationReason extension ExternalDependency { /// When explaining incremental decisions, it helps to know why a particular external dependency - /// was investigated. + /// caused invalidation. public enum InvalidationReason: String, CustomStringConvertible { /// An `import` of this file was added to the source code. case added @@ -356,7 +356,7 @@ extension ExternalDependency { /// The imported file has changed. case changed - /// Used when testing invalidation + /// Used when testing case testing public var description: String { rawValue } diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index 30f89697d..f662bd7a8 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -37,12 +37,13 @@ import SwiftOptions /// The phase when the graph was created. Used to help diagnose later failures let creationPhase: Phase - /// Minimize the number of file system modification-time queries. - private var externalDependencyModTimeCache = [ExternalDependency: Bool]() + fileprivate var currencyCache: ExternalDependencyCurrencyCache public init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup, _ phase: Phase ) { + self.currencyCache = ExternalDependencyCurrencyCache( + info.fileSystem, buildStartTime: info.buildStartTime) self.info = info self.dotFileWriter = info.emitDependencyDotFileAfterEveryImport ? DependencyGraphDotFileWriter(info) @@ -54,11 +55,21 @@ import SwiftOptions extension ModuleDependencyGraph { public enum Phase { - case - buildingWithoutAPrior, - updatingFromAPrior, - updatingAfterCompilation, - buildingAfterEachCompilation + /// Building a graph from swiftdeps files + case buildingWithoutAPrior + + /// Building a graph by reading a prior graph + /// and updating for changed external dependencies + case updatingFromAPrior + + /// Updating a graph from a `swiftdeps` file for an input that was just compiled + /// Or, updating a graph from a `swiftmodule` file (i.e. external dependency) that was transitively found to be + /// added as the `swiftdeps` file was processed. + case updatingAfterCompilation + + /// This is a clean incremental build. All inputs are being compiled and after each compilation, + /// the graph is being built from the new `swiftdeps` file. + case buildingAfterEachCompilation var isUpdating: Bool { switch self { @@ -69,41 +80,10 @@ extension ModuleDependencyGraph { } } - var shouldNewExternalDependenciesTriggerInvalidation: Bool { - switch self { - case .buildingWithoutAPrior: - // Reading graph from a swiftdeps file, - // so every incremental external dependency found will be new to the - // graph. Don't invalidate just 'cause it's new. - return false - - case .buildingAfterEachCompilation: - // Will be compiling every file, so no need to invalidate based on - // found external dependencies. - return false - - // Reading a swiftdeps file after a compilation. - // A new external dependency represents an addition. - // So must invalidate based on it. - case .updatingAfterCompilation: - return true - - case .updatingFromAPrior: - // If the graph was read from priors, - // then any new external dependency must also be an addition. - return true - } - } - - var isCompilingAllInputsNoMatterWhat: Bool { - switch self { - case .buildingAfterEachCompilation: - return true - case .buildingWithoutAPrior, .updatingFromAPrior, .updatingAfterCompilation: - return false - } + var isWholeGraphPresent: Bool { + self != .buildingWithoutAPrior } - } + } } // MARK: - Building from swiftdeps @@ -126,7 +106,8 @@ extension ModuleDependencyGraph { func collectNodesInvalidatedByChangedOrAddedExternals() -> DirectlyInvalidatedNodeSet { fingerprintedExternalDependencies.reduce(into: DirectlyInvalidatedNodeSet()) { invalidatedNodes, fed in - invalidatedNodes.formUnion(self.integrateExternal(.known(fed))) + invalidatedNodes.formUnion(self.incrementallyFindNodesInvalidated( + by: ExternalIntegrand(fed, shouldBeIn: self))) } } @@ -252,24 +233,24 @@ extension ModuleDependencyGraph { /// As an optimization, only return the nodes that have not been already traced, because the traced nodes /// will have already been used to schedule jobs to run. public func collectUntracedNodes( - from fingerprintedExternalDependency: FingerprintedExternalDependency, + thatUse externalDefs: FingerprintedExternalDependency, _ why: ExternalDependency.InvalidationReason ) -> DirectlyInvalidatedNodeSet { // These nodes will depend on the *interface* of the external Decl. let key = DependencyKey( aspect: .interface, - designator: .externalDepend(fingerprintedExternalDependency.externalDependency)) + designator: .externalDepend(externalDefs.externalDependency)) // DependencySource is OK as a nil placeholder because it's only used to find // the corresponding implementation node and there won't be any for an // external dependency node. let node = Node(key: key, - fingerprint: fingerprintedExternalDependency.fingerprint, + fingerprint: externalDefs.fingerprint, dependencySource: nil) let untracedUses = DirectlyInvalidatedNodeSet( nodeFinder .uses(of: node) .filter({ use in use.isUntraced })) - info.reporter?.reportInvalidated(untracedUses, by: fingerprintedExternalDependency.externalDependency, why) + info.reporter?.reportInvalidated(untracedUses, by: externalDefs.externalDependency, why) return untracedUses } @@ -330,94 +311,139 @@ extension ModuleDependencyGraph { extension ModuleDependencyGraph { /// The kinds of external dependencies available to integrate. enum ExternalIntegrand { - /// A `known` integrand is known to be present in the graph and requires - /// only a mod-time check to determine if it is up to date. - case known(FingerprintedExternalDependency) - /// An `unknown` integrand is not, up to this point, known to the dependency - /// graph. This models the addition of an import that is discovered during - /// the incremental build. - case unknown(FingerprintedExternalDependency) + /// An `old` integrand is one that, when found, is known to already be in ``ModuleDependencyGraph/fingerprintedExternalDependencies`` + case old(FingerprintedExternalDependency) + /// A `new` integrand is one that, when found was not already in ``ModuleDependencyGraph/fingerprintedExternalDependencies`` + case new(FingerprintedExternalDependency) + + init(_ fed: FingerprintedExternalDependency, + in graph: ModuleDependencyGraph ) { + self = graph.fingerprintedExternalDependencies.insert(fed).inserted + ? .old(fed) + : .new(fed) + } - var externalDependency: FingerprintedExternalDependency { - switch self { - case .known(let fed): return fed - case .unknown(let fed): return fed - } + init(_ fed: FingerprintedExternalDependency, + shouldBeIn graph: ModuleDependencyGraph ) { + assert(graph.fingerprintedExternalDependencies.contains(fed)) + self = .old(fed) } - var isKnown: Bool { + + var externalDependency: FingerprintedExternalDependency { switch self { - case .known(_): return true - case .unknown(_): return false + case .new(let fed), .old(let fed): return fed } } } + /// Find the nodes *directly* invaliadated by some external dependency + /// + /// This function does not do the transitive closure; that is left to the callers. + func findNodesInvalidated( + by integrand: ExternalIntegrand + ) -> DirectlyInvalidatedNodeSet { + // If the whole graph isn't present yet, the driver must not integrate + // incrementally because it would have to integrate the same `swiftmodule` + // repeatedy for each new `swiftdeps` read. + self.info.isCrossModuleIncrementalBuildEnabled && self.phase.isWholeGraphPresent + ? incrementallyFindNodesInvalidated(by: integrand) + : indiscriminatelyFindNodesInvalidated(by: integrand) + } + /// Collects the nodes invalidated by a change to the given external /// dependency after integrating it into the dependency graph. + /// Use best-effort to integrate incrementally, by reading the `swiftmodule` file. /// /// This function does not do the transitive closure; that is left to the /// callers. /// /// - Parameter integrand: The external dependency to integrate. /// - Returns: The set of module dependency graph nodes invalidated by integration. - func integrateExternal( - _ integrand: ExternalIntegrand + func incrementallyFindNodesInvalidated( + by integrand: ExternalIntegrand ) -> DirectlyInvalidatedNodeSet { - guard let whyInvalidate = self.invalidationReason(for: integrand) else { - return DirectlyInvalidatedNodeSet() - } + assert(self.info.isCrossModuleIncrementalBuildEnabled) - if self.info.isCrossModuleIncrementalBuildEnabled { - if let ii = integrateIncrementalImport(of: integrand.externalDependency, whyInvalidate) { - return ii - } + guard let whyIntegrate = whyIncrementallyFindNodesInvalidated(by: integrand) else { + return DirectlyInvalidatedNodeSet() } + return integrateIncrementalImport(of: integrand.externalDependency, whyIntegrate) + ?? indiscriminatelyFindNodesInvalidated(by: integrand) + } - // If we're compiling everything anyways, there's no need to trace. - // FIXME: Seems like - // 1) We could set this flag a lot earlier in some cases - // 2) It should apply to incremental imports as well. - guard !self.phase.isCompilingAllInputsNoMatterWhat else { - return DirectlyInvalidatedNodeSet() + /// Collects the nodes invalidated by a change to the given external + /// dependency after integrating it into the dependency graph. + /// + /// Do not try to be incremental; do not read the `swiftmodule` file. + /// + /// This function does not do the transitive closure; that is left to the + /// callers. + /// If called when incremental imports is enabled, it's a fallback. + /// + /// - Parameter integrand: The external dependency to integrate. + /// - Returns: The set of module dependency graph nodes invalidated by integration. + private func indiscriminatelyFindNodesInvalidated( + by integrand: ExternalIntegrand + ) -> DirectlyInvalidatedNodeSet { + if let reason = whyIndiscriminatelyFindNodesInvalidated(by: integrand) { + return collectUntracedNodes(thatUse: integrand.externalDependency, reason) } - return collectUntracedNodes(from: integrand.externalDependency, whyInvalidate) + return DirectlyInvalidatedNodeSet() } - /// Figure out the reason to invalidate or process a dependency. + /// Figure out the reason to integrate, (i.e. process) a dependency that will be read and integrated. /// /// Even if invalidation won't be reported to the caller, a new or added /// incremental external dependencies may require integration in order to /// transitively close them, (e.g. if an imported module imports a module). - private func invalidationReason( - for fed: ExternalIntegrand + /// + /// - Parameter fed: The external dependency, with fingerprint and origin info to be integrated + /// - Returns: nil if no integration is needed, or else why the integration is happening + private func whyIncrementallyFindNodesInvalidated(by integrand: ExternalIntegrand ) -> ExternalDependency.InvalidationReason? { - let isNewToTheGraph = !fed.isKnown && fingerprintedExternalDependencies.insert(fed.externalDependency).inserted - if self.phase.shouldNewExternalDependenciesTriggerInvalidation && isNewToTheGraph { + switch integrand { + case .new: return .added - } - - if self.hasFileChanged(fed.externalDependency.externalDependency) { + case .old where self.currencyCache.isCurrent(integrand.externalDependency.externalDependency): + // The most current version is already in the graph + return nil + case .old: return .changed } - return nil } - private func hasFileChanged(_ externalDependency: ExternalDependency) -> Bool { - if let hasChanged = externalDependencyModTimeCache[externalDependency] { - return hasChanged - } - guard let depFile = externalDependency.path else { - return true + /// Compute the reason for (non-incrementally) invalidating nodes + /// + /// Parameter integrand: The exernal dependency causing the invalidation + /// Returns: nil if no invalidation is needed, otherwise the reason. + private func whyIndiscriminatelyFindNodesInvalidated(by integrand: ExternalIntegrand + ) -> ExternalDependency.InvalidationReason? { + switch self.phase { + case .buildingWithoutAPrior, .updatingFromAPrior: + // If the external dependency has changed, better recompile any dependents + return self.currencyCache.isCurrent(integrand.externalDependency.externalDependency) + ? nil : .changed + case .updatingAfterCompilation: + switch integrand { + case .new: + // An import was added, some other file that does not import this might (transitively) depend on it + // so do the invalidation. + return .added + case .old: + // Not new to the graph in general, and this file was already compiled, + // so no work needed. + return nil + } + case .buildingAfterEachCompilation: + // No need to do any invalidation; every file will be compiled anyway + return nil } - let fileModTime = (try? info.fileSystem.lastModificationTime(for: depFile)) ?? .distantFuture - let hasChanged = fileModTime >= info.buildStartTime - externalDependencyModTimeCache[externalDependency] = hasChanged - return hasChanged } /// Try to read and integrate an external dependency. - /// Return nil if it's not incremental, or if an error occurs. + /// + /// Return: nil if an error occurs, or the set of directly affected nodes. private func integrateIncrementalImport( of fed: FingerprintedExternalDependency, _ why: ExternalDependency.InvalidationReason @@ -433,8 +459,52 @@ extension ModuleDependencyGraph { dependencySource: source, into: self) info.reporter?.reportInvalidated(invalidatedNodes, by: fed.externalDependency, why) + // When doing incremental imports, never read the same swiftmodule twice + self.currencyCache.beCurrent(fed.externalDependency) return invalidatedNodes } + + /// Remember if an external dependency need not be integrated in order to avoid redundant work. + /// + /// If using incremental imports, a given build should not read the same `swiftmodule` twice: + /// Because when using incremental imports, the whole graph is present, a single read of a `swiftmodule` + /// can invalidate any input file that depends on a changed external declaration. + /// + /// If not using incremental imports, a given build may have to invalidate nodes more than once for the same `swiftmodule`: + /// For example, on a clean build, as each initial `swiftdeps` is integrated, if the file uses a changed `swiftmodule`, + /// iit must be scheduled for recompilation. Thus invalidation happens for every dependent input file. + fileprivate struct ExternalDependencyCurrencyCache { + private let fileSystem: FileSystem + private let buildStartTime: Date + private var currencyCache = [ExternalDependency: Bool]() + + init(_ fileSystem: FileSystem, buildStartTime: Date) { + self.fileSystem = fileSystem + self.buildStartTime = buildStartTime + } + + mutating func beCurrent(_ externalDependency: ExternalDependency) { + self.currencyCache[externalDependency] = true + } + + mutating func isCurrent(_ externalDependency: ExternalDependency) -> Bool { + if let cachedResult = self.currencyCache[externalDependency] { + return cachedResult + } + let uncachedResult = isCurrentWRTFileSystem(externalDependency) + self.currencyCache[externalDependency] = uncachedResult + return uncachedResult + } + + private func isCurrentWRTFileSystem(_ externalDependency: ExternalDependency) -> Bool { + if let depFile = externalDependency.path, + let fileModTime = try? self.fileSystem.lastModificationTime(for: depFile), + fileModTime < self.buildStartTime { + return true + } + return false + } + } } // MARK: - tracking traced nodes diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/Integrator.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/Integrator.swift index e31fe60b9..86d18b7f4 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/Integrator.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/Integrator.swift @@ -233,7 +233,8 @@ extension ModuleDependencyGraph.Integrator { private mutating func recordInvalidations( from externalDependency: FingerprintedExternalDependency ) { - let invalidated = destination.integrateExternal(.unknown(externalDependency)) + let integrand = ModuleDependencyGraph.ExternalIntegrand(externalDependency, in: destination) + let invalidated = destination.findNodesInvalidated(by: integrand) recordUsesOfSomeExternal(invalidated) } } diff --git a/Tests/SwiftDriverTests/ModuleDependencyGraphTests.swift b/Tests/SwiftDriverTests/ModuleDependencyGraphTests.swift index af2fdab44..2edc2bc84 100644 --- a/Tests/SwiftDriverTests/ModuleDependencyGraphTests.swift +++ b/Tests/SwiftDriverTests/ModuleDependencyGraphTests.swift @@ -955,14 +955,14 @@ extension ModuleDependencyGraph { -> [Int] { phase = .updatingAfterCompilation - let directlyIinvalidatedNodes = getInvalidatedNodesForSimulatedLoad( + let directlyInvalidatedNodes = getInvalidatedNodesForSimulatedLoad( swiftDepsIndex, dependencyDescriptions, interfaceHash, includePrivateDeps: includePrivateDeps, hadCompilationError: hadCompilationError) - return collectInputsUsingInvalidated(nodes: directlyIinvalidatedNodes) + return collectInputsUsingInvalidated(nodes: directlyInvalidatedNodes) .map { $0.mockID } } @@ -1006,7 +1006,7 @@ extension ModuleDependencyGraph { on fingerprintedExternalDependency: FingerprintedExternalDependency ) -> [TypedVirtualPath] { var foundSources = [TypedVirtualPath]() - for dependent in collectUntracedNodes(from: fingerprintedExternalDependency, .testing) { + for dependent in collectUntracedNodes(thatUse: fingerprintedExternalDependency, .testing) { let dependencySource = dependent.dependencySource! foundSources.append(dependencySource.typedFile) // findSwiftDepsToRecompileWhenWholeSwiftDepChanges is reflexive From 15c43ccc914ebc8a18a6dda1cab04dd4257cd277 Mon Sep 17 00:00:00 2001 From: David Ungar Date: Tue, 6 Jul 2021 19:03:02 -0700 Subject: [PATCH 03/16] adjustment --- .../ModuleDependencyGraph.swift | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index f662bd7a8..014b4e58c 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -81,7 +81,14 @@ extension ModuleDependencyGraph { } var isWholeGraphPresent: Bool { - self != .buildingWithoutAPrior + !isBuilding + } + + var isBuilding: Bool { + switch self { + case .buildingWithoutAPrior, .buildingAfterEachCompilation: return true + case .updatingFromAPrior, .updatingAfterCompilation: return false + } } } } @@ -425,16 +432,8 @@ extension ModuleDependencyGraph { return self.currencyCache.isCurrent(integrand.externalDependency.externalDependency) ? nil : .changed case .updatingAfterCompilation: - switch integrand { - case .new: - // An import was added, some other file that does not import this might (transitively) depend on it - // so do the invalidation. - return .added - case .old: - // Not new to the graph in general, and this file was already compiled, - // so no work needed. - return nil - } + // Since this file has been compiled anyway, no need + return nil case .buildingAfterEachCompilation: // No need to do any invalidation; every file will be compiled anyway return nil From 153507152d5b5beca3c7678927b7743c968e7f48 Mon Sep 17 00:00:00 2001 From: David Ungar Date: Tue, 6 Jul 2021 19:03:17 -0700 Subject: [PATCH 04/16] removal test --- .../IncrementalCompilationTests.swift | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift index 9db1a6e7d..3b12403b4 100644 --- a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift +++ b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift @@ -473,6 +473,7 @@ extension IncrementalCompilationTests { disablingIncrementalCannotReadBuildRecord createdGraphFromSwiftdeps findingBatchingCompiling("main", "other") + reading(deps: "main", "other") schedLinking } } @@ -655,6 +656,7 @@ extension IncrementalCompilationTests { notSchedulingDependentsNoEntry(newInput) skipping("main", "other") findingBatchingCompiling(newInput) + reading(deps: newInput) newDefinitionOfSourceFile(.interface, newInput) newDefinitionOfSourceFile(.implementation, newInput) newDefinitionOfTopLevelName(.interface, name: topLevelName, input: newInput) @@ -679,7 +681,7 @@ extension IncrementalCompilationTests { ) throws { let newInputPath = inputPath(basename: newInput) let driver = try doABuild( - "after after addition of \(newInput)", + "after restoration of \(newInput)", checkDiagnostics: true, extraArguments: [newInputPath.pathString], whenAutolinking: autolinkLifecycleExpectedDiags @@ -725,9 +727,11 @@ extension IncrementalCompilationTests { switch (removeInputFromInvocation, removeSwiftDepsFile) { case (false, false): readGraphAndSkipAll("main", "other", removedInput) - case - (true, false), - (true, true): + case (true, false): + disabledForRemoval(removedInput) + findingBatchingCompiling("main", "other") + linking + case (true, true): disabledForRemoval(removedInput) findingBatchingCompiling("main", "other") linking @@ -739,6 +743,7 @@ extension IncrementalCompilationTests { queuingInitial(removedInput) skipping("main", "other") findingBatchingCompiling(removedInput) + reading(deps: removedInput) schedulingPostCompileJobs linking skipped("main", "other") @@ -781,7 +786,13 @@ extension IncrementalCompilationTests { extraArguments: extraArguments, whenAutolinking: autolinkLifecycleExpectedDiags ) { - !removeInputFromInvocation || afterRestoringBadPriors ? readGraph : createdGraphFromSwiftdeps + if !removeInputFromInvocation || afterRestoringBadPriors { + readGraph + } + else { + reading(deps: inputs) + createdGraphFromSwiftdeps + } enablingCrossModule if !removedFileDependsOnChangedFile { @@ -794,6 +805,7 @@ extension IncrementalCompilationTests { notSchedulingDependentsUnknownChanges("main") skipping(unchangedInputs) findingBatchingCompiling("main") + reading(deps: "main") fingerprintChanged(.interface, "main") fingerprintChanged(.implementation, "main") @@ -815,6 +827,7 @@ extension IncrementalCompilationTests { schedulingInvalidated(affectedInputsInBuild) let affectedInputsInInvocationOrder = inputs.filter(affectedInputsInBuild.contains) findingBatchingCompiling(affectedInputsInInvocationOrder) + reading(deps: affectedInputsInInvocationOrder) schedLinking } } @@ -840,6 +853,7 @@ extension IncrementalCompilationTests { extraArguments: [], whenAutolinking: autolinkLifecycleExpectedDiags) { couldNotReadPriors + reading(deps: "main", "other") createdGraphFromSwiftdeps enablingCrossModule skippingAll("main", "other") @@ -1166,6 +1180,14 @@ extension DiagVerifiable { .warning("Will not do cross-module incremental builds, wrong version of priors; expected") } // MARK: - dependencies + @DiagsBuilder func reading(deps inputs: [String]) -> [Diagnostic.Message] { + for input in inputs { + "Incremental compilation: Reading deps from \(input).swift" + } + } + @DiagsBuilder func reading(deps inputs: String...) -> [Diagnostic.Message] { + reading(deps: inputs) + } @DiagsBuilder func fingerprintChanged(_ aspect: DependencyKey.DeclAspect, _ input: String) -> [Diagnostic.Message] { "Incremental compilation: Fingerprint changed for \(aspect) of source file from \(input).swiftdeps in \(input).swift" } From 920860014405ac47f96504bac7d99d01474b951b Mon Sep 17 00:00:00 2001 From: David Ungar Date: Wed, 7 Jul 2021 11:54:03 -0700 Subject: [PATCH 05/16] incremental import tests fix --- .../ModuleDependencyGraph.swift | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index 014b4e58c..98098e7bc 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -79,18 +79,7 @@ extension ModuleDependencyGraph { return true } } - - var isWholeGraphPresent: Bool { - !isBuilding - } - - var isBuilding: Bool { - switch self { - case .buildingWithoutAPrior, .buildingAfterEachCompilation: return true - case .updatingFromAPrior, .updatingAfterCompilation: return false - } - } - } + } } // MARK: - Building from swiftdeps @@ -350,10 +339,7 @@ extension ModuleDependencyGraph { func findNodesInvalidated( by integrand: ExternalIntegrand ) -> DirectlyInvalidatedNodeSet { - // If the whole graph isn't present yet, the driver must not integrate - // incrementally because it would have to integrate the same `swiftmodule` - // repeatedy for each new `swiftdeps` read. - self.info.isCrossModuleIncrementalBuildEnabled && self.phase.isWholeGraphPresent + self.info.isCrossModuleIncrementalBuildEnabled ? incrementallyFindNodesInvalidated(by: integrand) : indiscriminatelyFindNodesInvalidated(by: integrand) } From 95cc262fc69a54cd5e54687205af9ea3537c685d Mon Sep 17 00:00:00 2001 From: David Ungar Date: Wed, 7 Jul 2021 12:01:05 -0700 Subject: [PATCH 06/16] fixed up tests --- Tests/SwiftDriverTests/IncrementalCompilationTests.swift | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift index 3b12403b4..9f05ab5e7 100644 --- a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift +++ b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift @@ -525,6 +525,7 @@ extension IncrementalCompilationTests { skipping("main") readGraph findingBatchingCompiling("other") + reading(deps: "other") schedLinking skipped("main") } @@ -551,6 +552,7 @@ extension IncrementalCompilationTests { readGraph schedulingChangedInitialQueuing("main", "other") findingBatchingCompiling("main", "other") + reading(deps: "main", "other") schedLinking } } @@ -579,6 +581,7 @@ extension IncrementalCompilationTests { notSchedulingDependentsUnknownChanges("main") skipping("other") findingBatchingCompiling("main") + reading(deps: "main") fingerprintChanged(.interface, "main") fingerprintChanged(.implementation, "main") trace { @@ -588,6 +591,7 @@ extension IncrementalCompilationTests { } queuingLaterSchedInvalBatchLink("other") findingBatchingCompiling("other") + reading(deps: "other") schedLinking } } @@ -626,6 +630,7 @@ extension IncrementalCompilationTests { addingToBatchThenForming("main", "other") schedulingPostCompileJobs compiling("main", "other") + reading(deps: "main", "other") linking } } @@ -902,6 +907,7 @@ extension IncrementalCompilationTests { readGraph schedulingChangedInitialQueuing("main", "other") findingBatchingCompiling("main", "other") + reading(deps: "main", "other") schedulingPostCompileJobs linking } From 715b76b668ded75331213bf5fb802be1ce15e7b2 Mon Sep 17 00:00:00 2001 From: David Ungar Date: Wed, 7 Jul 2021 14:10:18 -0700 Subject: [PATCH 07/16] Ensure no swiftmodule is read twice --- .../DependencySource.swift | 2 +- .../CompiledSourceCollector.swift | 22 +++++++++++++++++++ .../IncrementalCompilationTests.swift | 2 +- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift index f1e2f9b6e..d6ddbdda2 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift @@ -55,7 +55,7 @@ extension DependencySource { ) -> SourceFileDependencyGraph? { guard let fileToRead = fileToRead(info: info) else {return nil} do { - info.reporter?.report("Reading deps from \(description)") + info.reporter?.report("Reading dependencies from \(description)") return try SourceFileDependencyGraph.read(from: fileToRead, on: info.fileSystem) } catch { diff --git a/Tests/IncrementalTestFramework/CompiledSourceCollector.swift b/Tests/IncrementalTestFramework/CompiledSourceCollector.swift index 68e400086..9125c1505 100644 --- a/Tests/IncrementalTestFramework/CompiledSourceCollector.swift +++ b/Tests/IncrementalTestFramework/CompiledSourceCollector.swift @@ -21,6 +21,8 @@ import TestUtilities /// - seealso: Test struct CompiledSourceCollector { private var collectedCompiledBasenames = [String]() + private var collectedReadDependenciesInOrder = [String]() + private var collectedReadDependencies = Set() private func getCompiledBasenames(from d: Diagnostic) -> [String] { let dd = d.description @@ -38,9 +40,29 @@ struct CompiledSourceCollector { .compactMap {String($0)} } + private func getReadDependencies(from d: Diagnostic) -> String? { + let dd = d.description + guard let startOfReading = dd.range(of: "Reading dependencies ")?.upperBound + else { + return nil + } + return String(dd.suffix(from: startOfReading)) + } + + private mutating func appendReadDependency(_ dep: String) { + collectedReadDependenciesInOrder.append(dep) + let wasNew = collectedReadDependencies.insert(dep).inserted + guard wasNew || dep.hasSuffix(FileType.swift.rawValue) + else { + XCTFail("Swiftmodule \(dep) read twice") + return + } + } + /// Process a diagnistic mutating func handle(diagnostic d: Diagnostic) { collectedCompiledBasenames.append(contentsOf: getCompiledBasenames(from: d)) + getReadDependencies(from: d).map {appendReadDependency($0)} } /// Returns the basenames of the compiled files, e.g. for `/a/b/foo.swift`, returns `foo.swift`. diff --git a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift index 9f05ab5e7..6b210d3a2 100644 --- a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift +++ b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift @@ -1188,7 +1188,7 @@ extension DiagVerifiable { // MARK: - dependencies @DiagsBuilder func reading(deps inputs: [String]) -> [Diagnostic.Message] { for input in inputs { - "Incremental compilation: Reading deps from \(input).swift" + "Incremental compilation: Reading dependencies from \(input).swift" } } @DiagsBuilder func reading(deps inputs: String...) -> [Diagnostic.Message] { From 0212ba7e62cde3d1f1d7a04f5c6ed23f536499bf Mon Sep 17 00:00:00 2001 From: David Ungar Date: Wed, 7 Jul 2021 20:35:33 -0700 Subject: [PATCH 08/16] retrench to soundness --- .../ModuleDependencyGraph.swift | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index 98098e7bc..5444d1618 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -79,6 +79,17 @@ extension ModuleDependencyGraph { return true } } + + var isWholeGraphPresent: Bool { + !isBuilding + } + + var isBuilding: Bool { + switch self { + case .buildingWithoutAPrior, .buildingAfterEachCompilation: return true + case .updatingFromAPrior, .updatingAfterCompilation: return false + } + } } } @@ -339,7 +350,12 @@ extension ModuleDependencyGraph { func findNodesInvalidated( by integrand: ExternalIntegrand ) -> DirectlyInvalidatedNodeSet { - self.info.isCrossModuleIncrementalBuildEnabled + // If the whole graph isn't present yet, the driver must not integrate + // incrementally because it would have to integrate the same `swiftmodule` + // repeatedy for each new `swiftdeps` read so that external + // fingerprint changes would invalidate matching nodes in the + // yet-to-be-read swiftdeps. + self.info.isCrossModuleIncrementalBuildEnabled && self.phase.isWholeGraphPresent ? incrementallyFindNodesInvalidated(by: integrand) : indiscriminatelyFindNodesInvalidated(by: integrand) } From 25e7e6825d5c9d703b38acc054c1096fada06685 Mon Sep 17 00:00:00 2001 From: David Ungar Date: Wed, 7 Jul 2021 21:33:12 -0700 Subject: [PATCH 09/16] cannot be incremental if no print --- .../IncrementalCompilation/ModuleDependencyGraph.swift | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index 5444d1618..c594be88d 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -113,7 +113,7 @@ extension ModuleDependencyGraph { func collectNodesInvalidatedByChangedOrAddedExternals() -> DirectlyInvalidatedNodeSet { fingerprintedExternalDependencies.reduce(into: DirectlyInvalidatedNodeSet()) { invalidatedNodes, fed in - invalidatedNodes.formUnion(self.incrementallyFindNodesInvalidated( + invalidatedNodes.formUnion(self.findNodesInvalidated( by: ExternalIntegrand(fed, shouldBeIn: self))) } } @@ -355,7 +355,11 @@ extension ModuleDependencyGraph { // repeatedy for each new `swiftdeps` read so that external // fingerprint changes would invalidate matching nodes in the // yet-to-be-read swiftdeps. - self.info.isCrossModuleIncrementalBuildEnabled && self.phase.isWholeGraphPresent + // + // If the integrand has no fingerprint, it's academic, cannot integrate it incrementally. + self.info.isCrossModuleIncrementalBuildEnabled && + self.phase.isWholeGraphPresent && + integrand.externalDependency.fingerprint != nil ? incrementallyFindNodesInvalidated(by: integrand) : indiscriminatelyFindNodesInvalidated(by: integrand) } From 595605cde36eaffb3e7c34f12887ec5ba39c0eb4 Mon Sep 17 00:00:00 2001 From: David Ungar Date: Wed, 7 Jul 2021 21:58:42 -0700 Subject: [PATCH 10/16] fix typo bug --- .../IncrementalCompilation/ModuleDependencyGraph.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index c594be88d..e906cd8e9 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -326,8 +326,8 @@ extension ModuleDependencyGraph { init(_ fed: FingerprintedExternalDependency, in graph: ModuleDependencyGraph ) { self = graph.fingerprintedExternalDependencies.insert(fed).inserted - ? .old(fed) - : .new(fed) + ? .new(fed) + : .old(fed) } init(_ fed: FingerprintedExternalDependency, From 04df7b743270b88951ed06fa0e0cd5d542948c7a Mon Sep 17 00:00:00 2001 From: David Ungar Date: Wed, 7 Jul 2021 22:01:16 -0700 Subject: [PATCH 11/16] guard against recursion --- .../IncrementalCompilation/ModuleDependencyGraph.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index e906cd8e9..dcdc99456 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -459,13 +459,13 @@ extension ModuleDependencyGraph { else { return nil } + // When doing incremental imports, never read the same swiftmodule twice + self.currencyCache.beCurrent(fed.externalDependency) let invalidatedNodes = Integrator.integrate( from: unserializedDepGraph, dependencySource: source, into: self) info.reporter?.reportInvalidated(invalidatedNodes, by: fed.externalDependency, why) - // When doing incremental imports, never read the same swiftmodule twice - self.currencyCache.beCurrent(fed.externalDependency) return invalidatedNodes } From dd747e130a2ca0eab95314c2877c424a91d92041 Mon Sep 17 00:00:00 2001 From: David Ungar Date: Wed, 7 Jul 2021 22:25:06 -0700 Subject: [PATCH 12/16] Don't use swiftdeps when II is enabled and there is no prior. --- .../IncrementalDependencyAndInputSetup.swift | 19 ++++++--- .../ModuleDependencyGraph.swift | 39 +++++++------------ .../MockingIncrementalCompilation.swift | 2 +- 3 files changed, 29 insertions(+), 31 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift b/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift index 8974f2b39..41f7d106e 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift @@ -230,7 +230,13 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup { } guard let graph = graphIfPresent else { - return buildInitialGraphFromSwiftDepsAndCollectInputsInvalidatedByChangedExternals() + // Do not fall back to `buildInitialGraphFromSwiftDepsAndCollectInputsInvalidatedByChangedExternals` + // because it would be unsound to read a `swiftmodule` file with only a partial set of integrated `swiftdeps`. + // A fingerprint change in such a `swiftmodule` would not be able to propagate and invalidate a use + // in a as-yet-unread swiftdeps file. + // + // Instead, just compile everything. It's OK to be unsound then because every file will be compiled anyway. + return bulidEmptyGraphAndCompileEverything() } graph.dotFileWriter?.write(graph) @@ -257,10 +263,7 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup { private func buildInitialGraphFromSwiftDepsAndCollectInputsInvalidatedByChangedExternals() -> (ModuleDependencyGraph, TransitivelyInvalidatedInputSet)? { - guard let graph = ModuleDependencyGraph(self, .buildingWithoutAPrior) - else { - return nil - } + let graph = ModuleDependencyGraph(self, .buildingFromSwiftDeps) var inputsInvalidatedByChangedExternals = TransitivelyInvalidatedInputSet() for input in sourceFiles.currentInOrder { guard let invalidatedInputs = @@ -273,4 +276,10 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup { reporter?.report("Created dependency graph from swiftdeps files") return (graph, inputsInvalidatedByChangedExternals) } + + private func bulidEmptyGraphAndCompileEverything() + -> (ModuleDependencyGraph, TransitivelyInvalidatedInputSet) { + let graph = ModuleDependencyGraph(self, .buildingAfterEachCompilation) + return (graph, TransitivelyInvalidatedInputSet(sourceFiles.currentInOrder)) + } } diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index dcdc99456..998cfea75 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -39,15 +39,15 @@ import SwiftOptions fileprivate var currencyCache: ExternalDependencyCurrencyCache - public init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup, + public init(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup, _ phase: Phase ) { self.currencyCache = ExternalDependencyCurrencyCache( info.fileSystem, buildStartTime: info.buildStartTime) self.info = info self.dotFileWriter = info.emitDependencyDotFileAfterEveryImport - ? DependencyGraphDotFileWriter(info) - : nil + ? DependencyGraphDotFileWriter(info) + : nil self.phase = phase self.creationPhase = phase } @@ -55,8 +55,8 @@ import SwiftOptions extension ModuleDependencyGraph { public enum Phase { - /// Building a graph from swiftdeps files - case buildingWithoutAPrior + /// Building a graph from swiftdeps files, will never do this if incremental imports are enabled. + case buildingFromSwiftDeps /// Building a graph by reading a prior graph /// and updating for changed external dependencies @@ -73,7 +73,7 @@ extension ModuleDependencyGraph { var isUpdating: Bool { switch self { - case .buildingWithoutAPrior, .buildingAfterEachCompilation: + case .buildingFromSwiftDeps, .buildingAfterEachCompilation: return false case .updatingAfterCompilation, .updatingFromAPrior: return true @@ -86,7 +86,7 @@ extension ModuleDependencyGraph { var isBuilding: Bool { switch self { - case .buildingWithoutAPrior, .buildingAfterEachCompilation: return true + case .buildingFromSwiftDeps, .buildingAfterEachCompilation: return true case .updatingFromAPrior, .updatingAfterCompilation: return false } } @@ -350,16 +350,8 @@ extension ModuleDependencyGraph { func findNodesInvalidated( by integrand: ExternalIntegrand ) -> DirectlyInvalidatedNodeSet { - // If the whole graph isn't present yet, the driver must not integrate - // incrementally because it would have to integrate the same `swiftmodule` - // repeatedy for each new `swiftdeps` read so that external - // fingerprint changes would invalidate matching nodes in the - // yet-to-be-read swiftdeps. - // // If the integrand has no fingerprint, it's academic, cannot integrate it incrementally. - self.info.isCrossModuleIncrementalBuildEnabled && - self.phase.isWholeGraphPresent && - integrand.externalDependency.fingerprint != nil + self.info.isCrossModuleIncrementalBuildEnabled && integrand.externalDependency.fingerprint != nil ? incrementallyFindNodesInvalidated(by: integrand) : indiscriminatelyFindNodesInvalidated(by: integrand) } @@ -377,6 +369,8 @@ extension ModuleDependencyGraph { by integrand: ExternalIntegrand ) -> DirectlyInvalidatedNodeSet { assert(self.info.isCrossModuleIncrementalBuildEnabled) + // Better not be reading swiftdeps one-by-one for a selective compilation + precondition(self.phase != .buildingFromSwiftDeps) guard let whyIntegrate = whyIncrementallyFindNodesInvalidated(by: integrand) else { return DirectlyInvalidatedNodeSet() @@ -433,7 +427,7 @@ extension ModuleDependencyGraph { private func whyIndiscriminatelyFindNodesInvalidated(by integrand: ExternalIntegrand ) -> ExternalDependency.InvalidationReason? { switch self.phase { - case .buildingWithoutAPrior, .updatingFromAPrior: + case .buildingFromSwiftDeps, .updatingFromAPrior: // If the external dependency has changed, better recompile any dependents return self.currencyCache.isCurrent(integrand.externalDependency.externalDependency) ? nil : .changed @@ -631,12 +625,9 @@ extension ModuleDependencyGraph { /// The optionality of the contents lets the ``ModuleDependencyGraph/isForRemovedInput`` check to be cached. public private(set) var potentiallyUsedNodes: [Node?] = [] - init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup) { + init(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup) { self.fileSystem = info.fileSystem - guard let graph = ModuleDependencyGraph(info, .updatingFromAPrior) - else { - return nil - } + let graph = ModuleDependencyGraph(info, .updatingFromAPrior) self.graph = graph } @@ -765,9 +756,7 @@ extension ModuleDependencyGraph { } } - guard var visitor = Visitor(info) else { - return nil - } + var visitor = Visitor(info) try Bitcode.read(bytes: data, using: &visitor) guard let major = visitor.majorVersion, let minor = visitor.minorVersion, diff --git a/Tests/SwiftDriverTests/Helpers/MockingIncrementalCompilation.swift b/Tests/SwiftDriverTests/Helpers/MockingIncrementalCompilation.swift index ce7a3377b..c5c30a07d 100644 --- a/Tests/SwiftDriverTests/Helpers/MockingIncrementalCompilation.swift +++ b/Tests/SwiftDriverTests/Helpers/MockingIncrementalCompilation.swift @@ -174,7 +174,7 @@ struct MockModuleDependencyGraphCreator { } func mockUpAGraph() -> ModuleDependencyGraph { - ModuleDependencyGraph(info, .buildingWithoutAPrior)! + ModuleDependencyGraph(info, .buildingFromSwiftDeps) } } From c38e07df3a98d38a6bcdd9c15ba2e6ccbfa8d190 Mon Sep 17 00:00:00 2001 From: David Ungar Date: Thu, 8 Jul 2021 17:43:55 -0700 Subject: [PATCH 13/16] Fixup clean build and tests --- .../FirstWaveComputer.swift | 14 +- .../IncrementalDependencyAndInputSetup.swift | 2 +- .../IncrementalCompilationTests.swift | 125 ++++++++++++------ 3 files changed, 90 insertions(+), 51 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift b/Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift index d57e20ae3..ac15be76a 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift @@ -131,13 +131,13 @@ extension IncrementalCompilationState.FirstWaveComputer { } } - let inputsHavingMalformedDependencySources = - sourceFiles.currentInOrder.filter { sourceFile in - !moduleDependencyGraph.containsNodes(forSourceFile: sourceFile) - } + let inputsMissingFromGraph = sourceFiles.currentInOrder.filter { sourceFile in + !moduleDependencyGraph.containsNodes(forSourceFile: sourceFile) + } - if let reporter = reporter { - for input in inputsHavingMalformedDependencySources { + if let reporter = reporter, + moduleDependencyGraph.phase == .buildingFromSwiftDeps { + for input in inputsMissingFromGraph { reporter.report("Has malformed dependency source; will queue", input) } } @@ -156,7 +156,7 @@ extension IncrementalCompilationState.FirstWaveComputer { let definitelyRequiredInputs = Set(changedInputs.map({ $0.filePath }) + inputsInvalidatedByExternals + - inputsHavingMalformedDependencySources + + inputsMissingFromGraph + inputsMissingOutputs) if let reporter = reporter { for scheduledInput in sortByCommandLineOrder(definitelyRequiredInputs) { diff --git a/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift b/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift index 41f7d106e..3b932a423 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift @@ -280,6 +280,6 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup { private func bulidEmptyGraphAndCompileEverything() -> (ModuleDependencyGraph, TransitivelyInvalidatedInputSet) { let graph = ModuleDependencyGraph(self, .buildingAfterEachCompilation) - return (graph, TransitivelyInvalidatedInputSet(sourceFiles.currentInOrder)) + return (graph, TransitivelyInvalidatedInputSet()) } } diff --git a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift index 6b210d3a2..71ca21799 100644 --- a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift +++ b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift @@ -655,7 +655,6 @@ extension IncrementalCompilationTests { enablingCrossModule maySkip("main", "other") schedulingNew(newInput) - hasMalformed(newInput) missing(newInput) queuingInitial(newInput) notSchedulingDependentsNoEntry(newInput) @@ -784,55 +783,71 @@ extension IncrementalCompilationTests { let inputs = ["main", "other"] + (removeInputFromInvocation ? [] : [removedInput]) let extraArguments = removeInputFromInvocation ? [] : [inputPath(basename: removedInput).pathString] - + let haveGraph = !removeInputFromInvocation || afterRestoringBadPriors + let mainChanged = removedFileDependsOnChangedFile + let changedInputs = mainChanged ? ["main"] : [] + let unchangedInputs = inputs.filter {!changedInputs.contains($0)} + let driver = try doABuild( "restoring incrementality after removal of \(removedInput)", checkDiagnostics: true, extraArguments: extraArguments, whenAutolinking: autolinkLifecycleExpectedDiags ) { - if !removeInputFromInvocation || afterRestoringBadPriors { + if haveGraph { readGraph } - else { - reading(deps: inputs) - createdGraphFromSwiftdeps - } enablingCrossModule - if !removedFileDependsOnChangedFile { + if changedInputs.isEmpty && haveGraph { skippingAll(inputs) - } else { - schedulingChanged("main") - let unchangedInputs = inputs.filter {$0 != "main"} + } + else { + let firstWave = haveGraph ? changedInputs : inputs + let omittedFromFirstWave = haveGraph ? unchangedInputs : [] + let forcedInFirstWave = haveGraph ? [] : inputs + schedulingChanged(changedInputs) maySkip(unchangedInputs) - queuingInitial("main") - notSchedulingDependentsUnknownChanges("main") - skipping(unchangedInputs) - findingBatchingCompiling("main") - reading(deps: "main") - - fingerprintChanged(.interface, "main") - fingerprintChanged(.implementation, "main") - - let affectedInputs = removeInputFromInvocation - ? ["other"] - : [removedInput, "other"] - for input in affectedInputs { - trace { - TraceStep(.interface, source: "main") - TraceStep(.interface, .topLevel(name: "foo"), "main") - TraceStep(.implementation, .sourceFileProvide(name: "\(input).swiftdeps"), - input == removedInput && afterRestoringBadPriors - ? nil : input) + queuingInitial(firstWave) + notSchedulingDependentsUnknownChanges(changedInputs) + skipping(omittedFromFirstWave) + findingBatchingCompiling(firstWave) + reading(deps: firstWave) + if !haveGraph { + for (input, name) in [("main", "foo"), ("other", "bar")] { + newDefinitionOfSourceFile(.interface, input) + newDefinitionOfSourceFile(.implementation, input) + newDefinitionOfTopLevelName(.interface, name: name, input: input) + newDefinitionOfTopLevelName(.implementation, name: name, input: input) + } + } + else { + fingerprintChanged(.interface, "main") + fingerprintChanged(.implementation, "main") + + let affectedInputs = removeInputFromInvocation && !afterRestoringBadPriors + ? ["other"] + : [removedInput, "other"] + for input in affectedInputs { + trace { + TraceStep(.interface, source: "main") + TraceStep(.interface, .topLevel(name: "foo"), "main") + TraceStep(.implementation, .sourceFileProvide(name: "\(input).swiftdeps"), + input == removedInput && afterRestoringBadPriors + ? nil : input) + } } + if removeInputFromInvocation && afterRestoringBadPriors { + failedToFindSource(removedInput) + failedToReadSomeSource(compiling: "main") + } + let affectedInputsInBuild = affectedInputs.filter(inputs.contains) + queuingLater(affectedInputsInBuild) + schedulingInvalidated(affectedInputsInBuild) + let affectedInputsInInvocationOrder = inputs.filter(affectedInputsInBuild.contains) + findingBatchingCompiling(affectedInputsInInvocationOrder) + reading(deps: affectedInputsInInvocationOrder) } - let affectedInputsInBuild = affectedInputs.filter(inputs.contains) - queuingLater(affectedInputsInBuild) - schedulingInvalidated(affectedInputsInBuild) - let affectedInputsInInvocationOrder = inputs.filter(affectedInputsInBuild.contains) - findingBatchingCompiling(affectedInputsInInvocationOrder) - reading(deps: affectedInputsInInvocationOrder) schedLinking } } @@ -857,11 +872,22 @@ extension IncrementalCompilationTests { checkDiagnostics: true, extraArguments: [], whenAutolinking: autolinkLifecycleExpectedDiags) { - couldNotReadPriors - reading(deps: "main", "other") - createdGraphFromSwiftdeps - enablingCrossModule - skippingAll("main", "other") + couldNotReadPriors + enablingCrossModule + maySkip("main", "other") + queuingInitial("main", "other") + findingBatchingCompiling("main", "other") + reading(deps: "main") + newDefinitionOfSourceFile(.interface, "main") + newDefinitionOfSourceFile(.implementation, "main") + newDefinitionOfTopLevelName(.interface, name: "foo", input: "main") + newDefinitionOfTopLevelName(.implementation, name: "foo", input: "main") + reading(deps: "other") + newDefinitionOfSourceFile(.interface, "other") + newDefinitionOfSourceFile(.implementation, "other") + newDefinitionOfTopLevelName(.interface, name: "bar", input: "other") + newDefinitionOfTopLevelName(.implementation, name: "bar", input: "other") + schedLinking } } @@ -1208,8 +1234,21 @@ extension DiagVerifiable { @DiagsBuilder func foundDependent(of defInput: String, compiling useInput: String) -> [Diagnostic.Message] { "Incremental compilation: Found dependent of \(defInput).swift: {compile: \(useInput).o <= \(useInput).swift}" } - @DiagsBuilder func hasMalformed(_ newInput: String) -> [Diagnostic.Message] { - "Incremental compilation: Has malformed dependency source; will queue {compile: \(newInput).o <= \(newInput).swift}" + @DiagsBuilder func hasMalformed(_ inputs: [String]) -> [Diagnostic.Message] { + for newInput in inputs { + "Incremental compilation: Has malformed dependency source; will queue {compile: \(newInput).o <= \(newInput).swift}" + } + } + @DiagsBuilder func hasMalformed(_ inputs: String...) -> [Diagnostic.Message] { + hasMalformed(inputs) + } + @DiagsBuilder func invalidatedExternally(_ inputs: [String]) -> [Diagnostic.Message] { + for input in inputs { + "Incremental compilation: Invalidated externally; will queue {compile: \(input).o <= \(input).swift}" + } + } + @DiagsBuilder func invalidatedExternally(_ inputs: String...) -> [Diagnostic.Message] { + invalidatedExternally(inputs) } @DiagsBuilder func failedToFindSource(_ input: String) -> [Diagnostic.Message] { .warning("Failed to find source file '\(input).swift' in command line, recovering with a full rebuild. Next build will be incremental.") From a7030d7a5f602426e4b2a2c8d7c027ab5b9d2a25 Mon Sep 17 00:00:00 2001 From: David Ungar Date: Thu, 8 Jul 2021 18:06:02 -0700 Subject: [PATCH 14/16] Address review. --- .../IncrementalCompilation/ModuleDependencyGraph.swift | 9 +++++---- .../CompiledSourceCollector.swift | 2 -- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index 998cfea75..e2e10588e 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -393,10 +393,11 @@ extension ModuleDependencyGraph { private func indiscriminatelyFindNodesInvalidated( by integrand: ExternalIntegrand ) -> DirectlyInvalidatedNodeSet { - if let reason = whyIndiscriminatelyFindNodesInvalidated(by: integrand) { - return collectUntracedNodes(thatUse: integrand.externalDependency, reason) + guard let reason = whyIndiscriminatelyFindNodesInvalidated(by: integrand) + else { + return DirectlyInvalidatedNodeSet() } - return DirectlyInvalidatedNodeSet() + return collectUntracedNodes(thatUse: integrand.externalDependency, reason) } /// Figure out the reason to integrate, (i.e. process) a dependency that will be read and integrated. @@ -471,7 +472,7 @@ extension ModuleDependencyGraph { /// /// If not using incremental imports, a given build may have to invalidate nodes more than once for the same `swiftmodule`: /// For example, on a clean build, as each initial `swiftdeps` is integrated, if the file uses a changed `swiftmodule`, - /// iit must be scheduled for recompilation. Thus invalidation happens for every dependent input file. + /// it must be scheduled for recompilation. Thus invalidation happens for every dependent input file. fileprivate struct ExternalDependencyCurrencyCache { private let fileSystem: FileSystem private let buildStartTime: Date diff --git a/Tests/IncrementalTestFramework/CompiledSourceCollector.swift b/Tests/IncrementalTestFramework/CompiledSourceCollector.swift index 9125c1505..4b7d756e5 100644 --- a/Tests/IncrementalTestFramework/CompiledSourceCollector.swift +++ b/Tests/IncrementalTestFramework/CompiledSourceCollector.swift @@ -21,7 +21,6 @@ import TestUtilities /// - seealso: Test struct CompiledSourceCollector { private var collectedCompiledBasenames = [String]() - private var collectedReadDependenciesInOrder = [String]() private var collectedReadDependencies = Set() private func getCompiledBasenames(from d: Diagnostic) -> [String] { @@ -50,7 +49,6 @@ struct CompiledSourceCollector { } private mutating func appendReadDependency(_ dep: String) { - collectedReadDependenciesInOrder.append(dep) let wasNew = collectedReadDependencies.insert(dep).inserted guard wasNew || dep.hasSuffix(FileType.swift.rawValue) else { From 7ac2178255db6b7157ad2591d06beb3f00907a5a Mon Sep 17 00:00:00 2001 From: David Ungar Date: Fri, 9 Jul 2021 11:13:57 -0700 Subject: [PATCH 15/16] Fix test to reflect prior culling PR that landed --- .../SwiftDriverTests/IncrementalCompilationTests.swift | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift index 71ca21799..21dde2a54 100644 --- a/Tests/SwiftDriverTests/IncrementalCompilationTests.swift +++ b/Tests/SwiftDriverTests/IncrementalCompilationTests.swift @@ -825,9 +825,9 @@ extension IncrementalCompilationTests { fingerprintChanged(.interface, "main") fingerprintChanged(.implementation, "main") - let affectedInputs = removeInputFromInvocation && !afterRestoringBadPriors - ? ["other"] - : [removedInput, "other"] + let affectedInputs = removeInputFromInvocation + ? ["other"] + : [removedInput, "other"] for input in affectedInputs { trace { TraceStep(.interface, source: "main") @@ -837,10 +837,6 @@ extension IncrementalCompilationTests { ? nil : input) } } - if removeInputFromInvocation && afterRestoringBadPriors { - failedToFindSource(removedInput) - failedToReadSomeSource(compiling: "main") - } let affectedInputsInBuild = affectedInputs.filter(inputs.contains) queuingLater(affectedInputsInBuild) schedulingInvalidated(affectedInputsInBuild) From e2ecf2293585d6a9157bdde85dc16343d577f4ce Mon Sep 17 00:00:00 2001 From: David Ungar Date: Fri, 9 Jul 2021 12:43:59 -0700 Subject: [PATCH 16/16] Fix typos --- .../IncrementalDependencyAndInputSetup.swift | 2 +- .../IncrementalCompilation/ModuleDependencyGraph.swift | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift b/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift index 3b932a423..f86bea871 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift @@ -206,7 +206,7 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup { return readPriorGraphAndCollectInputsInvalidatedByChangedOrAddedExternals() } // Every external is added, but don't want to compile an unchanged input that has an import - // so just changed, not changedOrAdded + // so just changed, not changedOrAdded. return buildInitialGraphFromSwiftDepsAndCollectInputsInvalidatedByChangedExternals() } diff --git a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift index e2e10588e..4c827198a 100644 --- a/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift +++ b/Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift @@ -424,7 +424,7 @@ extension ModuleDependencyGraph { /// Compute the reason for (non-incrementally) invalidating nodes /// /// Parameter integrand: The exernal dependency causing the invalidation - /// Returns: nil if no invalidation is needed, otherwise the reason. + /// - Returns: nil if no invalidation is needed, otherwise the reason. private func whyIndiscriminatelyFindNodesInvalidated(by integrand: ExternalIntegrand ) -> ExternalDependency.InvalidationReason? { switch self.phase { @@ -443,7 +443,7 @@ extension ModuleDependencyGraph { /// Try to read and integrate an external dependency. /// - /// Return: nil if an error occurs, or the set of directly affected nodes. + /// - Returns: nil if an error occurs, or the set of directly affected nodes. private func integrateIncrementalImport( of fed: FingerprintedExternalDependency, _ why: ExternalDependency.InvalidationReason