[Dependency Scanning] Serialize visible Clang module sets for each by-name queried Clang module

Re-use them on successive incremental scanning actions

Author: artemcm

include/swift/AST/ModuleDependencies.h

@@ -277,9 +277,8 @@ class ModuleDependencyInfoStorageBase {
   /// The macro dependencies.
   std::map<std::string, MacroPluginDependency> macroDependencies;
 
-  /// A list of Clang modules that are visible to this Swift module. This
-  /// includes both direct Clang modules as well as transitive Clang
-  /// module dependencies when they are exported
+  /// A list of Clang modules that are visible to this Swift module
+  /// as re-exported modular includes of its bridging header.
   std::vector<std::string> bridgingHeaderVisibleClangModules;
 
   /// ModuleDependencyInfo is finalized (with all transitive dependencies

include/swift/DependencyScan/SerializedModuleDependencyCacheFormat.h

@@ -41,7 +41,7 @@ using llvm::BCVBR;
 const unsigned char MODULE_DEPENDENCY_CACHE_FORMAT_SIGNATURE[] = {'I', 'M', 'D','C'};
 const unsigned MODULE_DEPENDENCY_CACHE_FORMAT_VERSION_MAJOR = 10;
 /// Increment this on every change.
-const unsigned MODULE_DEPENDENCY_CACHE_FORMAT_VERSION_MINOR = 4;
+const unsigned MODULE_DEPENDENCY_CACHE_FORMAT_VERSION_MINOR = 5;
 
 /// Various identifiers in this format will rely on having their strings mapped
 /// using this ID.
@@ -81,6 +81,7 @@ using FileIDArrayIDField = IdentifierIDField;
 using ContextHashIDField = IdentifierIDField;
 using ModuleCacheKeyIDField = IdentifierIDField;
 using ImportArrayIDField = IdentifierIDField;
+using VisibleModulesArrayIDField = IdentifierIDField;
 using LinkLibrariesArrayIDField = IdentifierIDField;
 using MacroDependenciesArrayIDField = IdentifierIDField;
 using SearchPathArrayIDField = IdentifierIDField;
@@ -109,6 +110,7 @@ enum {
   MACRO_DEPENDENCY_ARRAY_NODE,
   SEARCH_PATH_NODE,
   SEARCH_PATH_ARRAY_NODE,
+  VISIBLE_MODULES_NODE,
   IMPORT_STATEMENT_NODE,
   IMPORT_STATEMENT_ARRAY_NODE,
   OPTIONAL_IMPORT_STATEMENT_ARRAY_NODE,
@@ -187,6 +189,14 @@ using SearchPathLayout =
 using SearchPathArrayLayout =
     BCRecordLayout<SEARCH_PATH_ARRAY_NODE, IdentifierIDArryField>;
 
+// A record capturing information about all Clang modules visible
+// from a given named Clang module dependency query
+using VisibleModulesLayout =
+  BCRecordLayout<VISIBLE_MODULES_NODE,        // ID
+                 IdentifierIDField,           // moduleIdentifier
+                 VisibleModulesArrayIDField   // visibleModulesArrayIdentifier
+                 >;
+
 // A record capturing information about a given 'import' statement
 // captured in a dependency node, including its source location.
 using ImportStatementLayout =
@@ -199,6 +209,7 @@ using ImportStatementLayout =
                    IsExportedImport,             // isExported
                    AccessLevelField              // accessLevel
                    >;
+
 using ImportStatementArrayLayout =
     BCRecordLayout<IMPORT_STATEMENT_ARRAY_NODE, IdentifierIDArryField>;
 using OptionalImportStatementArrayLayout =

lib/AST/ModuleDependencies.cpp

@@ -879,6 +879,10 @@ void ModuleDependenciesCache::updateDependency(
 void ModuleDependenciesCache::removeDependency(ModuleDependencyID moduleID) {
   auto &map = getDependenciesMap(moduleID.Kind);
   map.erase(moduleID.ModuleName);
+  // If we are removing a Clang module which was queried by-name
+  // in a prior scan, we must re-compute its set of visible modules.
+  if (moduleID.Kind == ModuleDependencyKind::Clang)
+    clangModulesVisibleFromNamedLookup.erase(moduleID.ModuleName);
 }
 
 void ModuleDependenciesCache::setImportedSwiftDependencies(
@@ -973,14 +977,11 @@ llvm::StringSet<> ModuleDependenciesCache::getAllVisibleClangModules(
   }
   for (const auto &clangDepID :
        findKnownDependency(moduleID).getImportedClangDependencies()) {
-    // FIXME: This scenario should not occur
-    // assert(hasVisibleClangModulesFrom(clangDepID.ModuleName));
-    if (hasVisibleClangModulesFromLookup(clangDepID.ModuleName)) {
-      auto visibleModulesViaImport =
-          getVisibleClangModulesFromLookup(clangDepID.ModuleName);
-      result.insert(visibleModulesViaImport.begin(),
-                    visibleModulesViaImport.end());
-    }
+    assert(hasVisibleClangModulesFromLookup(clangDepID.ModuleName));
+    auto visibleModulesViaImport =
+      getVisibleClangModulesFromLookup(clangDepID.ModuleName);
+    result.insert(visibleModulesViaImport.begin(),
+                  visibleModulesViaImport.end());
   }
   return result;
 }

lib/DependencyScan/ModuleDependencyCacheSerialization.cpp

@@ -411,6 +411,23 @@ bool ModuleDependenciesCacheDeserializer::readGraph(
       break;
     }
 
+    case VISIBLE_MODULES_NODE: {
+      uint64_t identifierID;
+      uint64_t valueArrayID;
+      VisibleModulesLayout::readRecord(Scratch, identifierID, valueArrayID);
+      auto moduleName = getIdentifier(identifierID);
+      if (!moduleName)
+        llvm::report_fatal_error(
+            "Bad visible modules info: no module name");
+      auto values = getStringArray(valueArrayID);
+      if (!values)
+        llvm::report_fatal_error(
+            "Bad visible modules info: modules");
+
+      cache.clangModulesVisibleFromNamedLookup[moduleName.value()] = values.value();
+      break;
+    }
+
     case IMPORT_STATEMENT_NODE: {
       unsigned importIdentifierID, bufferIdentifierID;
       unsigned lineNumber, columnNumber;
@@ -1056,6 +1073,7 @@ enum ModuleIdentifierArrayKind : uint8_t {
   BridgingHeaderBuildCommandLine,
   NonPathCommandLine,
   FileDependencies,
+  VisibleClangModulesFromLookup,
   LastArrayKind
 };
 
@@ -1166,6 +1184,7 @@ class ModuleDependenciesCacheSerializer {
   unsigned writeSearchPaths(const SwiftBinaryModuleDependencyStorage &dependencyInfo);
   void writeSearchPathsArray(unsigned startIndex, unsigned count);
 
+  void writeVisibleClangModuleInfo(const ModuleDependenciesCache &cache);
   void writeImportStatementInfos(const ModuleDependenciesCache &cache);
   unsigned writeImportStatementInfos(const ModuleDependencyInfo &dependencyInfo,
                                      bool optional);
@@ -1435,6 +1454,21 @@ void ModuleDependenciesCacheSerializer::writeSearchPathsArray(unsigned startInde
       Out, ScratchRecord, AbbrCodes[SearchPathArrayLayout::Code], vec);
 }
 
+void ModuleDependenciesCacheSerializer::writeVisibleClangModuleInfo(
+    const ModuleDependenciesCache &cache) {
+  using namespace graph_block;
+  for (const auto &entry : cache.clangModulesVisibleFromNamedLookup) {
+    auto moduleID =
+        ModuleDependencyID{entry.getKey().str(), ModuleDependencyKind::Clang};
+    VisibleModulesLayout::emitRecord(
+        Out, ScratchRecord, AbbrCodes[VisibleModulesLayout::Code],
+        getIdentifier(moduleID.ModuleName),
+        getIdentifierArrayID(
+            moduleID,
+            ModuleIdentifierArrayKind::VisibleClangModulesFromLookup));
+  }
+}
+
 void ModuleDependenciesCacheSerializer::writeImportStatementInfos(
     const ModuleDependenciesCache &cache) {
   unsigned lastImportInfoIndex = 0;
@@ -1798,6 +1832,15 @@ unsigned ModuleDependenciesCacheSerializer::getOptionalImportStatementsArrayID(
 void ModuleDependenciesCacheSerializer::collectStringsAndArrays(
     const ModuleDependenciesCache &cache) {
   addIdentifier(cache.scannerContextHash);
+
+  for (const auto &entry : cache.clangModulesVisibleFromNamedLookup) {
+    auto moduleName = entry.getKey().str();
+    addIdentifier(moduleName);
+    addStringArray({moduleName, ModuleDependencyKind::Clang},
+                   ModuleIdentifierArrayKind::VisibleClangModulesFromLookup,
+                   entry.second);
+  }
+
   for (auto kind = ModuleDependencyKind::FirstKind;
        kind != ModuleDependencyKind::LastKind; ++kind) {
     auto modMap = cache.getDependenciesMap(kind);
@@ -1973,6 +2016,7 @@ void ModuleDependenciesCacheSerializer::writeInterModuleDependenciesCache(
   registerRecordAbbr<MacroDependencyArrayLayout>();
   registerRecordAbbr<SearchPathLayout>();
   registerRecordAbbr<SearchPathArrayLayout>();
+  registerRecordAbbr<VisibleModulesLayout>();
   registerRecordAbbr<ImportStatementLayout>();
   registerRecordAbbr<ImportStatementArrayLayout>();
   registerRecordAbbr<OptionalImportStatementArrayLayout>();
@@ -1998,6 +2042,9 @@ void ModuleDependenciesCacheSerializer::writeInterModuleDependenciesCache(
   // Write the arrays
   writeArraysOfIdentifiers();
 
+  // Write the cached sets of visible modules
+  writeVisibleClangModuleInfo(cache);
+
   // Write all the import statement info
   writeImportStatementInfos(cache);
 

lib/DependencyScan/ModuleDependencyScanner.cpp

@@ -1197,25 +1197,31 @@ void ModuleDependencyScanner::processBatchClangModuleQueryResult(
         auto dependencyID =
             ModuleDependencyID{moduleIdentifier, ModuleDependencyKind::Clang};
 
-        // Add visible Clang modules for the queried dependency
+        // A successful named query will have returned a set of visible modules
         if (lookupResult.visibleModules.contains(moduleIdentifier)) {
           ScanDiagnosticReporter.registerNamedClangDependency();
           DependencyCache.setVisibleClangModulesFromLookup(
               dependencyID, lookupResult.visibleModules.at(moduleIdentifier));
-        }
-
-        if (DependencyCache.hasDependency(dependencyID))
           resolvedClangDependenciesMap[moduleID].insert(dependencyID);
-        else if (lookupResult.discoveredDependencyInfos.contains(
-                     moduleIdentifier)) {
-          resolvedClangDependenciesMap[moduleID].insert(dependencyID);
-          auto dependencyInfo = lookupResult.discoveredDependencyInfos.at(
-              moduleImport.importIdentifier);
-          allDiscoveredClangModules.insert(dependencyID);
-          DependencyCache.recordClangDependency(
-              dependencyInfo, ScanASTContext.Diags, [this](auto &clangDep) {
-                return bridgeClangModuleDependency(clangDep);
-              });
+
+          // If this module was not seen before as a transitive dependency
+          // of another lookup, record it into the cache
+          if (lookupResult.discoveredDependencyInfos.contains(
+                  moduleIdentifier)) {
+            allDiscoveredClangModules.insert(dependencyID);
+            DependencyCache.recordClangDependency(
+                lookupResult.discoveredDependencyInfos.at(
+                    moduleImport.importIdentifier),
+                ScanASTContext.Diags, [this](auto &clangDep) {
+                  return bridgeClangModuleDependency(clangDep);
+                });
+          } else {
+            // If the query produced a set of visible modules but not
+            // a `ModuleDeps` info for the queried module itself, then
+            // it has to have been included in the set of already-seen
+            // module dependencies from a prior query.
+            assert(DependencyCache.hasDependency(dependencyID));
+          }
         } else if (!optionalImport) {
           // Otherwise, we failed to resolve this dependency. We will try
           // again using the cache after all other imports have been

test/ScanDependencies/basic_query_metrics.swift

@@ -9,7 +9,7 @@
 // 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 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 Clang module dependencies queried by-name from a Swift client: '1'
 // CHECK: remark: Number of recorded Swift module dependencies: '2'
 // CHECK: remark: Number of recorded Clang module dependencies: '1'
 

test/ScanDependencies/duplicate_clang_queries_on_overlay.swift

@@ -0,0 +1,53 @@
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t/module-cache)
+// RUN: %empty-directory(%t/inputs)
+// RUN: split-file %s %t
+
+// RUN: %target-swift-frontend -scan-dependencies -module-name Test -module-cache-path %t/module-cache -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib %t/test.swift -o %t/deps.json -I %t/inputs -Rdependency-scan -no-parallel-scan &> %t/remarks.txt
+// RUN: cat %t/remarks.txt | %FileCheck %s
+
+// Ensure that although the Swift overlay dependency 'A' shares a dependency on 'B' and 'C'
+// it does not incur additional namedqueries for them.
+//
+// CHECK: remark: Number of Swift module queries: '9'
+// CHECK: remark: Number of named Clang module queries: '2'
+// CHECK: remark: Number of recorded Clang module dependencies queried by-name from a Swift client: '2'
+// CHECK: remark: Number of recorded Swift module dependencies: '1'
+// CHECK: remark: Number of recorded Clang module dependencies: '3'
+
+//--- test.swift
+import B
+import C
+public func test() {}
+
+//--- inputs/A.swiftinterface
+// swift-interface-format-version: 1.0
+// swift-module-flags: -module-name A -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib -user-module-version 1.0
+import B
+import C
+public func a() { }
+
+//--- inputs/A.h
+#include "A.h"
+void b(void);
+
+//--- inputs/B.h
+#include "A.h"
+void b(void);
+
+//--- inputs/C.h
+void c(void);
+
+//--- inputs/module.modulemap
+module A {
+  header "A.h"
+  export *
+}
+module B {
+  header "B.h"
+  export *
+}
+module C {
+  header "C.h"
+  export *
+}

test/ScanDependencies/module_deps_cache_reuse.swift

@@ -29,13 +29,9 @@ import SubE
 // CHECK-REMARK-SAVE: remark: Incremental module scan: Serializing module scanning dependency cache to:
 
 // 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'
-// 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'
-// CHECK-REMARK-LOAD: remark: Number of recorded Clang module dependencies queried by-name from a Swift client: '1'
+// CHECK-REMARK-LOAD: remark: Number of Swift module queries: '12'
+// CHECK-REMARK-LOAD: remark: Number of named Clang module queries: '0'
+// CHECK-REMARK-LOAD: remark: Number of recorded Clang module dependencies queried by-name from a Swift client: '0'
 // CHECK-REMARK-LOAD: remark: Number of recorded Swift module dependencies: '8'
 // CHECK-REMARK-LOAD: remark: Number of recorded Clang module dependencies: '7'