Add caching of unsuccessful Swift dependency lookups

Use it to avoid unnecessary re-queries of Swift dependencies during a given scanning action.

Author: artemcm

include/swift/AST/ModuleDependencies.h

@@ -1073,6 +1073,9 @@ class ModuleDependenciesCache {
 private:
   /// Discovered dependencies
   ModuleDependenciesKindMap ModuleDependenciesMap;
+  /// A set of module identifiers for which a scanning action failed
+  /// to discover a Swift module dependency
+  llvm::StringSet<> negativeSwiftDependencyCache;
   /// Set containing all of the Clang modules that have already been seen.
   llvm::DenseSet<clang::tooling::dependencies::ModuleID> alreadySeenClangModules;
   /// Name of the module under scan
@@ -1111,6 +1114,10 @@ class ModuleDependenciesCache {
   bool hasDependency(StringRef moduleName) const;
   /// Whether we have cached dependency information for the given Swift module.
   bool hasSwiftDependency(StringRef moduleName) const;
+  /// Whether we have previously failed a lookup of a Swift dependency for the
+  /// given identifier.
+  bool hasNegativeSwiftDependency(StringRef moduleName) const;
+
   /// Report the number of recorded Clang dependencies
   int numberOfClangDependencies() const;
   /// Report the number of recorded Swift dependencies
@@ -1232,6 +1239,8 @@ class ModuleDependenciesCache {
   void
   addVisibleClangModules(ModuleDependencyID moduleID,
                          const std::vector<std::string> &moduleNames);
+  /// Add an identifier to the set of failed Swift module queries
+  void recordFailedSwiftDependencyLookup(StringRef moduleIdentifier);
 
   StringRef getMainModuleName() const { return mainScanModuleName; }
 

lib/AST/ModuleDependencies.cpp

@@ -772,14 +772,17 @@ bool ModuleDependenciesCache::hasSwiftDependency(StringRef moduleName) const {
   return findSwiftDependency(moduleName).has_value();
 }
 
+bool ModuleDependenciesCache::hasNegativeSwiftDependency(StringRef moduleName) const {
+  return negativeSwiftDependencyCache.contains(moduleName);
+}
+
 int ModuleDependenciesCache::numberOfClangDependencies() const {
   return ModuleDependenciesMap.at(ModuleDependencyKind::Clang).size();
 }
 int ModuleDependenciesCache::numberOfSwiftDependencies() const {
   return ModuleDependenciesMap.at(ModuleDependencyKind::SwiftInterface).size() +
          ModuleDependenciesMap.at(ModuleDependencyKind::SwiftBinary).size();
 }
-
 void ModuleDependenciesCache::recordDependency(
     StringRef moduleName, ModuleDependencyInfo dependency) {
   auto dependenciesKind = dependency.getKind();
@@ -958,6 +961,11 @@ void ModuleDependenciesCache::addVisibleClangModules(
   updateDependency(moduleID, updatedDependencyInfo);
 }
 
+void ModuleDependenciesCache::recordFailedSwiftDependencyLookup(
+    StringRef moduleIdentifier) {
+  negativeSwiftDependencyCache.insert(moduleIdentifier);
+}
+
 llvm::StringSet<> &ModuleDependenciesCache::getVisibleClangModules(
     ModuleDependencyID moduleID) const {
   ASSERT(moduleID.Kind == ModuleDependencyKind::SwiftSource ||

lib/DependencyScan/ModuleDependencyScanner.cpp

@@ -26,6 +26,7 @@
 #include "swift/Basic/Defer.h"
 #include "swift/Basic/FileTypes.h"
 #include "swift/Basic/PrettyStackTrace.h"
+#include "swift/Basic/Statistic.h"
 #include "swift/ClangImporter/ClangImporter.h"
 #include "swift/Frontend/CompileJobCacheKey.h"
 #include "swift/Frontend/ModuleInterfaceLoader.h"
@@ -1347,6 +1348,13 @@ void ModuleDependencyScanner::resolveSwiftImportsForModule(
     // Avoid querying the underlying Clang module here
     if (moduleID.ModuleName == dependsOn.importIdentifier)
       continue;
+    // Avoid querying Swift module dependencies previously discovered
+    if (DependencyCache.hasSwiftDependency(dependsOn.importIdentifier))
+      continue;
+    // Avoid querying Swift module dependencies which have already produced
+    // in a negative (not found) result
+    if (DependencyCache.hasNegativeSwiftDependency(dependsOn.importIdentifier))
+      continue;
     ScanningThreadPool.async(
         scanForSwiftModuleDependency,
         getModuleImportIdentifier(dependsOn.importIdentifier),
@@ -1386,10 +1394,13 @@ void ModuleDependencyScanner::resolveSwiftImportsForModule(
                        moduleImport.importIdentifier))
           importedSwiftDependencies.insert(
               {moduleImport.importIdentifier, cachedInfo.value()->getKind()});
-        else
+        else {
           ScanDiagnosticReporter.diagnoseFailureOnOnlyIncompatibleCandidates(
                      moduleImport, lookupResult.incompatibleCandidates,
                      DependencyCache, std::nullopt);
+          DependencyCache
+            .recordFailedSwiftDependencyLookup(moduleImport.importIdentifier);
+        }
       };
 
   for (const auto &importInfo : moduleDependencyInfo.getModuleImports())
@@ -1533,10 +1544,9 @@ void ModuleDependencyScanner::resolveSwiftOverlayDependenciesForModule(
     auto moduleName = moduleIdentifier.str();
     {
       std::lock_guard<std::mutex> guard(lookupResultLock);
-      if (DependencyCache.hasDependency(moduleName,
-                                        ModuleDependencyKind::SwiftInterface) ||
-          DependencyCache.hasDependency(moduleName,
-                                        ModuleDependencyKind::SwiftBinary))
+      if (DependencyCache.hasDependency(moduleName, ModuleDependencyKind::SwiftInterface) ||
+          DependencyCache.hasDependency(moduleName, ModuleDependencyKind::SwiftBinary) ||
+          DependencyCache.hasNegativeSwiftDependency(moduleName))
         return;
     }
 

test/ScanDependencies/basic_query_metrics.swift

@@ -7,7 +7,7 @@
 // RUN: cat %t/remarks.txt | %FileCheck %s
 
 // Ensure that despite being a common dependency to multiple Swift modules, only 1 query is performed to find 'C'
-// CHECK: remark: Number of Swift module queries: '6'
+// CHECK: remark: Number of Swift module queries: '3'
 // CHECK: remark: Number of named Clang module queries: '1'
 // CHEKC: remark: Number of recorded Clang module dependencies queried by-name from a Swift client: '1'
 // CHECK: remark: Number of recorded Swift module dependencies: '2'

test/ScanDependencies/diagnose_dependency_cycle_source_target.swift

@@ -5,8 +5,9 @@
 
 // RUN: %FileCheck %s < %t/out.txt
 
-// CHECK: module dependency cycle: 'CycleOverlay (Source Target) -> CycleSwiftMiddle.swiftinterface -> CycleOverlay.swiftinterface'
+// CHECK: error: module dependency cycle: 'CycleOverlay (Source Target) -> CycleSwiftMiddle.swiftinterface -> CycleOverlay (Source Target)'
 // CHECK: Swift Overlay dependency of 'CycleSwiftMiddle' on 'CycleOverlay' via Clang module dependency: 'CycleSwiftMiddle.swiftinterface -> CycleClangMiddle.pcm -> CycleOverlay.pcm'
+// CHECK: source target 'CycleOverlay' shadowing a Swift module with the same name
 
 import CycleSwiftMiddle
 

test/ScanDependencies/module_deps_cache_reuse.swift

@@ -21,7 +21,7 @@ import E
 import G
 import SubE
 
-// CHECK-REMARK-SAVE: remark: Number of Swift module queries: '21'
+// CHECK-REMARK-SAVE: remark: Number of Swift module queries: '12'
 // CHECK-REMARK-SAVE: remark: Number of named Clang module queries: '6'
 // CHECK-REMARK-SAVE: remark: Number of recorded Clang module dependencies queried by-name from a Swift client: '6'
 // CHECK-REMARK-SAVE: remark: Number of recorded Swift module dependencies: '8'
@@ -31,7 +31,7 @@ import SubE
 // CHECK-REMARK-LOAD: remark: Incremental module scan: Re-using serialized module scanning dependency cache from:
 // 'SwiftShims', 'C' and 'B' are Clang modules which are directly imorted from Swift code in this test, without a corresponding Swift overlay module. Because we do not serialize negative Swift dependency lookup results, resolving a dependency on these modules involves a query for whether a Swift module under this name can be found. This query fails and the subsequent query for this identifier as a Clang dependency is then able to re-use the loaded serialized cache.
 
-// CHECK-REMARK-LOAD: remark: Number of Swift module queries: '7'
+// CHECK-REMARK-LOAD: remark: Number of Swift module queries: '3'
 // FIXME: Today, we do not serialize dependencies of the main source module which results in a lookup for 'C' even though
 // it is fully redundant.
 // CHECK-REMARK-LOAD: remark: Number of named Clang module queries: '1'