Merge pull request #751 from davidungar/5.5-clean-build-fix

[5.5, Incremental]: Fix clean build performance regression

Author: David Ungar

Sources/SwiftDriver/IncrementalCompilation/DependencyKey.swift

@@ -51,8 +51,8 @@ import TSCBasic
       return "non-path: '\(fileName)'"
     }
     switch path.extension {
-    case FileType.swiftModule.rawValue:
-      // Swift modules have an extra component at the end that is not descriptive
+    case FileType.swiftModule.rawValue, FileType.swiftInterface.rawValue:
+      // These have an extra component at the end that is not descriptive
       return path.parentDirectory.basename
     default:
       return path.basename

Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift

@@ -131,13 +131,14 @@ extension IncrementalCompilationState.FirstWaveComputer {
       }
     }
 
-    let inputsHavingMalformedDependencySources =
+    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 == .buildingWithoutAPrior {
+      for input in inputsMissingFromGraph {
         reporter.report("Has malformed dependency source; will queue", input)
       }
     }
@@ -156,7 +157,7 @@ extension IncrementalCompilationState.FirstWaveComputer {
     let definitelyRequiredInputs =
       Set(changedInputs.map({ $0.filePath }) +
             inputsInvalidatedByExternals +
-            inputsHavingMalformedDependencySources +
+            inputsMissingFromGraph +
             inputsMissingOutputs)
     if let reporter = reporter {
       for scheduledInput in sortByCommandLineOrder(definitelyRequiredInputs) {

Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift

@@ -124,7 +124,7 @@ extension IncrementalCompilationState {
     // Do not try to reuse a graph from a different compilation, so check
     // the build record.
     @_spi(Testing) public var readPriorsFromModuleDependencyGraph: Bool {
-      maybeBuildRecord != nil && options.contains(.readPriorsFromModuleDependencyGraph)
+      options.contains(.readPriorsFromModuleDependencyGraph)
     }
     @_spi(Testing) public var alwaysRebuildDependents: Bool {
       options.contains(.alwaysRebuildDependents)
@@ -233,10 +233,14 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup {
         warning: "Could not read \(dependencyGraphPath), will not do cross-module incremental builds")
       graphIfPresent = nil
     }
-    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.
+    // Do this by creating an empty graph here.
+    let graph = graphIfPresent ?? ModuleDependencyGraph(self, .buildingAfterEachCompilation)
     guard graph.populateInputDependencySourceMap() else {
       return nil
     }

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

@@ -168,7 +168,8 @@ extension ModuleDependencyGraph {
 // MARK: - Getting a graph read from priors ready to use
 extension ModuleDependencyGraph {
   func collectNodesInvalidatedByChangedOrAddedExternals() -> DirectlyInvalidatedNodeSet {
-    fingerprintedExternalDependencies.reduce(into: DirectlyInvalidatedNodeSet()) {
+    assert(info.isCrossModuleIncrementalBuildEnabled)
+    return fingerprintedExternalDependencies.reduce(into: DirectlyInvalidatedNodeSet()) {
       invalidatedNodes, fed in
       invalidatedNodes.formUnion (
         self.collectNodesInvalidatedByProcessing(fingerprintedExternalDependency: fed,
@@ -356,22 +357,23 @@ extension ModuleDependencyGraph {
     should: fed,
     whichIsNewToTheGraph: isNewToTheGraph,
     closeOverSwiftModulesIn: self)
-
-    let invalidatedNodesFromIncrementalExternal = whyIntegrateForClosure.flatMap { why in
-      collectNodesInvalidatedByAttemptingToProcess(why, fed)
-    }
-
-    guard let whyInvalidate = ExternalDependency.Why(
+    
+    // collectNodesInvalidatedByAttemptingToProcess will change the currency cache
+    // so get this reason now.
+    let whyInvalidate = ExternalDependency.Why(
       shouldUsesOf: fed,
       whichIsNewToTheGraph: isNewToTheGraph,
       beInvalidatedIn: self)
-    else {
-      return DirectlyInvalidatedNodeSet()
+
+    let invalidatedNodesFromIncrementalExternal = whyIntegrateForClosure.flatMap { why in
+      collectNodesInvalidatedByAttemptingToProcess(why, fed)
     }
 
     // If there was an error integrating the external dependency, or if it was not an incremental one,
     // return anything that uses that dependency.
-    return invalidatedNodesFromIncrementalExternal ?? collectUntracedNodesUsing(whyInvalidate, fed)
+    return invalidatedNodesFromIncrementalExternal
+    ?? whyInvalidate.map {collectUntracedNodesUsing($0, fed)}
+    ?? DirectlyInvalidatedNodeSet()
   }
 
  func hasFileChanged(of externalDependency: ExternalDependency
@@ -387,6 +389,10 @@ extension ModuleDependencyGraph {
     externalDependencyModTimeCache[externalDependency] = hasChanged
     return hasChanged
   }
+  
+  func beCurrent(_ externalDependency: ExternalDependency) {
+    externalDependencyModTimeCache[externalDependency] = false
+  }
 
   /// Try to read and integrate an external dependency.
   /// Return nil if it's not incremental, or if an error occurs.
@@ -399,6 +405,7 @@ extension ModuleDependencyGraph {
     else {
       return nil
     }
+    beCurrent(fed.externalDependency) // Don't read the same external twice
     let invalidatedNodes = Integrator.integrate(from: unserializedDepGraph, into: self)
     info.reporter?.reportInvalidated(invalidatedNodes, by: fed.externalDependency, why)
     return invalidatedNodes

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift

@@ -56,6 +56,7 @@ extension DependencySource {
   ) -> SourceFileDependencyGraph? {
     let graphIfPresent: SourceFileDependencyGraph?
     do {
+      reporter?.report("Reading dependencies from \(description)")
       graphIfPresent = try SourceFileDependencyGraph.read(
         from: self,
         on: fileSystem)

Tests/IncrementalTestFramework/CompiledSourceCollector.swift

@@ -21,6 +21,8 @@ import TestUtilities
 /// - seealso: Test
 struct CompiledSourceCollector {
   private var collectedCompiledBasenames = [String]()
+  private var collectedReadDependencies = Set<String>()
+
 
   private func getCompiledBasenames(from d: Diagnostic) -> [String] {
     let dd = d.description
@@ -37,10 +39,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) {
+    let wasNew = collectedReadDependencies.insert(dep).inserted
+    guard wasNew || dep.hasSuffix(FileType.swiftDeps.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`.

Tests/SwiftDriverTests/IncrementalCompilationTests.swift

@@ -159,6 +159,7 @@ extension IncrementalCompilationTests {
     expectNoDotFiles()
     try tryInitial(extraArguments: ["-driver-emit-fine-grained-dependency-dot-file-after-every-import"])
     expect(dotFilesFor: [
+      DependencyGraphDotFileWriter.moduleDependencyGraphBasename,
       "main.swiftdeps",
       DependencyGraphDotFileWriter.moduleDependencyGraphBasename,
       "other.swiftdeps",
@@ -322,14 +323,15 @@ extension IncrementalCompilationTests {
         "Enabling incremental cross-module building",
         "Incremental compilation: Incremental compilation could not read build record at",
         "Incremental compilation: Disabling incremental build: could not read build record",
-        "Incremental compilation: Created dependency graph from swiftdeps files",
         "Found 2 batchable jobs",
         "Forming into 1 batch",
         "Adding {compile: main.swift} to batch 0",
         "Adding {compile: other.swift} to batch 0",
         "Forming batch job from 2 constituents: main.swift, other.swift",
         "Starting Compiling main.swift, other.swift",
         "Finished Compiling main.swift, other.swift",
+        "Incremental compilation: Reading dependencies from main.swiftdeps",
+        "Incremental compilation: Reading dependencies from other.swiftdeps",
         "Incremental compilation: Scheduling all post-compile jobs because something was compiled",
         "Starting Linking theModule",
         "Finished Linking theModule",
@@ -378,6 +380,7 @@ extension IncrementalCompilationTests {
         "Forming batch job from 1 constituents: other.swift",
         "Starting Compiling other.swift",
         "Finished Compiling other.swift",
+        "Incremental compilation: Reading dependencies from other.swiftdeps",
         "Incremental compilation: Scheduling all post-compile jobs because something was compiled",
         "Starting Linking theModule",
         "Finished Linking theModule",
@@ -410,6 +413,8 @@ extension IncrementalCompilationTests {
         "Forming batch job from 2 constituents: main.swift, other.swift",
         "Starting Compiling main.swift, other.swift",
         "Finished Compiling main.swift, other.swift",
+        "Incremental compilation: Reading dependencies from main.swiftdeps",
+        "Incremental compilation: Reading dependencies from other.swiftdeps",
         "Incremental compilation: Scheduling all post-compile jobs because something was compiled",
         "Starting Linking theModule",
         "Finished Linking theModule",
@@ -439,6 +444,7 @@ extension IncrementalCompilationTests {
         "Forming batch job from 1 constituents: main.swift",
         "Starting Compiling main.swift",
         "Finished Compiling main.swift",
+        "Incremental compilation: Reading dependencies from main.swiftdeps",
         "Incremental compilation: Fingerprint changed for interface of source file main.swiftdeps in main.swiftdeps",
         "Incremental compilation: Fingerprint changed for implementation of source file main.swiftdeps in main.swiftdeps",
         "Incremental compilation: Traced: interface of source file main.swiftdeps in main.swift -> interface of top-level name 'foo' in main.swift -> implementation of source file other.swiftdeps in other.swift",
@@ -450,6 +456,7 @@ extension IncrementalCompilationTests {
         "Forming batch job from 1 constituents: other.swift",
         "Starting Compiling other.swift",
         "Finished Compiling other.swift",
+        "Incremental compilation: Reading dependencies from other.swiftdeps",
         "Incremental compilation: Scheduling all post-compile jobs because something was compiled",
         "Starting Linking theModule",
         "Finished Linking theModule",
@@ -485,6 +492,8 @@ extension IncrementalCompilationTests {
         "Incremental compilation: Scheduling all post-compile jobs because something was compiled",
         "Starting Compiling main.swift, other.swift",
         "Finished Compiling main.swift, other.swift",
+        "Incremental compilation: Reading dependencies from main.swiftdeps",
+        "Incremental compilation: Reading dependencies from other.swiftdeps",
         "Starting Linking theModule",
         "Finished Linking theModule",
       ],
@@ -511,6 +520,7 @@ extension IncrementalCompilationTests {
         .remark("Forming batch job from 1 constituents: main.swift"),
         .remark("Starting Compiling main.swift"),
         .remark("Finished Compiling main.swift"),
+        .remark("Incremental compilation: Reading dependencies from main.swiftdeps"),
         .remark("Incremental compilation: Fingerprint changed for interface of source file main.swiftdeps in main.swiftdeps"),
         .remark("Incremental compilation: Fingerprint changed for implementation of source file main.swiftdeps in main.swiftdeps"),
         .remark("Incremental compilation: Traced: interface of source file main.swiftdeps in main.swift -> interface of top-level name 'foo' in main.swift -> implementation of source file other.swiftdeps in other.swift"),
@@ -525,6 +535,7 @@ extension IncrementalCompilationTests {
         .remark("Forming batch job from 1 constituents: other.swift"),
         .remark("Starting Compiling other.swift"),
         .remark("Finished Compiling other.swift"),
+        .remark("Incremental compilation: Reading dependencies from other.swiftdeps"),
         .remark("Incremental compilation: Scheduling all post-compile jobs because something was compiled"),
         .remark("Starting Linking theModule"),
         .remark("Finished Linking theModule"),
@@ -607,6 +618,8 @@ extension IncrementalCompilationTests {
         "Forming batch job from 2 constituents: main.swift, other.swift",
         "Starting Compiling main.swift, other.swift",
         "Finished Compiling main.swift, other.swift",
+        "Incremental compilation: Reading dependencies from main.swiftdeps",
+        "Incremental compilation: Reading dependencies from other.swiftdeps",
         "Incremental compilation: Scheduling all post-compile jobs because something was compiled",
         "Starting Linking theModule",
         "Finished Linking theModule",