[Dependency Scanning] Do not persist cached Clang module dependencies between scans.

This change tweaks the 'GlobalModuleDependenciesCache', which persists across scanner invocations with the same 'DependencyScanningTool' to no longer cache discovered Clang modules.

Doing so felt like a premature optimization, and we should instead attempt to share as much state as possible by keeping around the actual Clang scanner's state, which performs its own caching. Caching discovered dependencies both in the Clang scanner instance, and in our own cache is much more error-prone - the Clang scanner has a richer context for what is okay and not okay to cache/re-use.

Instead, we still cache discovered Clang dependencies *within* a given scan, since those are discovered using a common Clang scanner instance and should be safe to keep for the duration of the scan.

This change should make it simpler to pin down the core functionality and correctness of the scanner.
Once we turn our attention to the scanner's performance, we can revisit this strategy and optimize the caching behaviour.

Author: artemcm

include/swift/AST/ModuleDependencies.h

@@ -527,7 +527,6 @@ class GlobalModuleDependenciesCache {
   GlobalModuleDependenciesCache(const GlobalModuleDependenciesCache &) = delete;
   GlobalModuleDependenciesCache &
   operator=(const GlobalModuleDependenciesCache &) = delete;
-
   virtual ~GlobalModuleDependenciesCache() {}
 
   void configureForTriple(std::string triple);
@@ -603,24 +602,18 @@ class ModuleDependenciesCache {
 private:
   GlobalModuleDependenciesCache &globalCache;
 
-  /// References to data in `globalCache` for dependencies accimulated during
+  /// References to data in `globalCache` for Swift dependencies and
+  /// `clangModuleDependencies` for Clang dependencies accimulated during
   /// the current scanning action.
   ModuleDependenciesKindRefMap ModuleDependenciesMap;
 
-  /// Additional information needed for Clang dependency scanning.
-  ClangModuleDependenciesCacheImpl *clangImpl = nullptr;
-
-  /// Name of the main Swift module of this scan
-  StringRef mainModuleName;
-  /// Underlying Clang module is seen differently by the main
-  /// module and by module clients. For this reason, we do not wish subsequent
-  /// scans to be able to re-use this dependency info and therefore we avoid
-  /// adding it to the global cache. The dependency storage is therefore tied
-  /// to this, local, cache.
-  std::unique_ptr<ModuleDependencies> underlyingClangModuleDependency = nullptr;
+  /// Discovered Clang modules are only cached locally.
+  llvm::StringMap<ModuleDependenciesVector> clangModuleDependencies;
 
   /// Function that will delete \c clangImpl properly.
   void (*clangImplDeleter)(ClangModuleDependenciesCacheImpl *) = nullptr;
+  /// Additional information needed for Clang dependency scanning.
+  ClangModuleDependenciesCacheImpl *clangImpl = nullptr;
 
   /// Free up the storage associated with the Clang implementation.
   void destroyClangImpl() {
@@ -647,8 +640,7 @@ class ModuleDependenciesCache {
                    Optional<ModuleDependenciesKind> kind) const;
 
 public:
-  ModuleDependenciesCache(GlobalModuleDependenciesCache &globalCache,
-                          StringRef mainModuleName);
+  ModuleDependenciesCache(GlobalModuleDependenciesCache &globalCache);
   ModuleDependenciesCache(const ModuleDependenciesCache &) = delete;
   ModuleDependenciesCache &operator=(const ModuleDependenciesCache &) = delete;
   virtual ~ModuleDependenciesCache() { destroyClangImpl(); }

include/swift/AST/ModuleLoader.h

@@ -19,13 +19,13 @@
 
 #include "swift/AST/Identifier.h"
 #include "swift/AST/Import.h"
-#include "swift/AST/ModuleDependencies.h"
 #include "swift/Basic/ArrayRefView.h"
 #include "swift/Basic/Fingerprint.h"
 #include "swift/Basic/LLVM.h"
 #include "swift/Basic/Located.h"
 #include "swift/Basic/SourceLoc.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/TinyPtrVector.h"
 #include "llvm/Support/VersionTuple.h"
 #include <system_error>
@@ -52,6 +52,7 @@ class NominalTypeDecl;
 class SourceFile;
 class TypeDecl;
 class CompilerInstance;
+enum class ModuleDependenciesKind : int8_t;
 
 enum class KnownProtocolKind : uint8_t;
 

include/swift/Sema/SourceLoader.h

@@ -13,7 +13,6 @@
 #ifndef SWIFT_SEMA_SOURCELOADER_H
 #define SWIFT_SEMA_SOURCELOADER_H
 
-#include "swift/AST/ModuleDependencies.h"
 #include "swift/AST/ModuleLoader.h"
 
 namespace swift {
@@ -94,14 +93,10 @@ class SourceLoader : public ModuleLoader {
     // Parsing populates the Objective-C method tables.
   }
 
-  Optional<ModuleDependencies> getModuleDependencies(
-      StringRef moduleName, ModuleDependenciesCache &cache,
-      InterfaceSubContextDelegate &delegate) override {
-    // FIXME: Implement?
-    return None;
-  }
+  Optional<ModuleDependencies>
+  getModuleDependencies(StringRef moduleName, ModuleDependenciesCache &cache,
+                        InterfaceSubContextDelegate &delegate) override;
 };
-
 }
 
 #endif

lib/AST/ModuleDependencies.cpp

@@ -388,6 +388,9 @@ GlobalModuleDependenciesCache::findSourceModuleDependency(
 
 bool GlobalModuleDependenciesCache::hasDependencies(
     StringRef moduleName, ModuleLookupSpecifics details) const {
+  assert(details.kind != ModuleDependenciesKind::Clang &&
+         "Attempting to query Clang dependency in persistent Dependency "
+         "Scanner Cache.");
   return findDependencies(moduleName, details).hasValue();
 }
 
@@ -397,6 +400,9 @@ GlobalModuleDependenciesCache::findAllDependenciesIrrespectiveOfSearchPaths(
   if (!kind) {
     for (auto kind = ModuleDependenciesKind::FirstKind;
          kind != ModuleDependenciesKind::LastKind; ++kind) {
+      if (kind == ModuleDependenciesKind::Clang)
+        continue;
+
       auto deps =
           findAllDependenciesIrrespectiveOfSearchPaths(moduleName, kind);
       if (deps.hasValue())
@@ -446,6 +452,10 @@ static std::string modulePathForVerification(const ModuleDependencies &module) {
 const ModuleDependencies *GlobalModuleDependenciesCache::recordDependencies(
     StringRef moduleName, ModuleDependencies dependencies) {
   auto kind = dependencies.getKind();
+  assert(kind != ModuleDependenciesKind::Clang &&
+         "Attempting to cache Clang dependency in persistent Dependency "
+         "Scanner Cache.");
+
   // Source-based dependencies are recorded independently of the invocation's
   // target triple.
   if (kind == swift::ModuleDependenciesKind::SwiftSource) {
@@ -482,6 +492,10 @@ const ModuleDependencies *GlobalModuleDependenciesCache::recordDependencies(
 const ModuleDependencies *GlobalModuleDependenciesCache::updateDependencies(
     ModuleDependencyID moduleID, ModuleDependencies dependencies) {
   auto kind = dependencies.getKind();
+  assert(kind != ModuleDependenciesKind::Clang &&
+         "Attempting to update Clang dependency in persistent Dependency "
+         "Scanner Cache.");
+
   // Source-based dependencies
   if (kind == swift::ModuleDependenciesKind::SwiftSource) {
     assert(SwiftSourceModuleDependenciesMap.count(moduleID.first) == 1 &&
@@ -517,9 +531,8 @@ ModuleDependenciesCache::getDependencyReferencesMap(
 }
 
 ModuleDependenciesCache::ModuleDependenciesCache(
-    GlobalModuleDependenciesCache &globalCache,
-    StringRef mainModuleName)
-    : globalCache(globalCache), mainModuleName(mainModuleName) {
+    GlobalModuleDependenciesCache &globalCache)
+    : globalCache(globalCache) {
   for (auto kind = ModuleDependenciesKind::FirstKind;
        kind != ModuleDependenciesKind::LastKind; ++kind) {
     ModuleDependenciesMap.insert(
@@ -572,12 +585,29 @@ void ModuleDependenciesCache::recordDependencies(
   // The underlying Clang module needs to be cached in this invocation,
   // but should not make it to the global cache since it will look slightly
   // differently for clients of this module than it does for the module itself.
-  const ModuleDependencies* recordedDependencies;
-  if (moduleName == mainModuleName && dependencies.isClangModule()) {
-    underlyingClangModuleDependency = std::make_unique<ModuleDependencies>(std::move(dependencies));
-    recordedDependencies = underlyingClangModuleDependency.get();
+  const ModuleDependencies *recordedDependencies;
+  if (dependencies.getKind() == ModuleDependenciesKind::Clang) {
+    auto *clangDep = dependencies.getAsClangModule();
+    assert(clangDep && "Unexpected NULL Clang dependency.");
+    // Cache may already have a dependency for this module
+    if (clangModuleDependencies.count(moduleName) != 0) {
+      // Do not record duplicate dependencies.
+      auto newModulePath = clangDep->moduleMapFile;
+      for (auto &existingDeps : clangModuleDependencies[moduleName]) {
+        if (modulePathForVerification(existingDeps) == newModulePath)
+          return;
+      }
+      clangModuleDependencies[moduleName].emplace_back(std::move(dependencies));
+      recordedDependencies = clangModuleDependencies[moduleName].end() - 1;
+    } else {
+      clangModuleDependencies.insert(
+          {moduleName, ModuleDependenciesVector{std::move(dependencies)}});
+      recordedDependencies = &(clangModuleDependencies[moduleName].front());
+    }
+
   } else
-    recordedDependencies = globalCache.recordDependencies(moduleName, dependencies);
+    recordedDependencies =
+        globalCache.recordDependencies(moduleName, dependencies);
 
   auto &map = getDependencyReferencesMap(dependenciesKind);
   assert(map.count(moduleName) == 0 && "Already added to map");

lib/AST/ModuleLoader.cpp

@@ -18,6 +18,7 @@
 #include "swift/AST/DiagnosticsCommon.h"
 #include "swift/AST/FileUnit.h"
 #include "swift/AST/ModuleLoader.h"
+#include "swift/AST/ModuleDependencies.h"
 #include "swift/Basic/FileTypes.h"
 #include "swift/Basic/Platform.h"
 #include "swift/Basic/SourceManager.h"

lib/ClangImporter/ImporterImpl.h

@@ -53,6 +53,7 @@
 #include <functional>
 #include <set>
 #include <unordered_set>
+#include <unordered_map>
 #include <vector>
 
 namespace llvm {

lib/DependencyScan/DependencyScanningTool.cpp

@@ -73,8 +73,7 @@ DependencyScanningTool::getDependencies(
   auto Instance = std::move(*InstanceOrErr);
 
   // Local scan cache instance, wrapping the shared global cache.
-  ModuleDependenciesCache cache(*SharedCache,
-                                Instance->getMainModule()->getNameStr());
+  ModuleDependenciesCache cache(*SharedCache);
   // Execute the scanning action, retrieving the in-memory result
   auto DependenciesOrErr = performModuleScan(*Instance.get(), cache);
   if (DependenciesOrErr.getError())
@@ -114,8 +113,7 @@ DependencyScanningTool::getDependencies(
   auto Instance = std::move(*InstanceOrErr);
 
   // Local scan cache instance, wrapping the shared global cache.
-  ModuleDependenciesCache cache(*SharedCache,
-                                Instance->getMainModule()->getNameStr());
+  ModuleDependenciesCache cache(*SharedCache);
   auto BatchScanResults = performBatchModuleScan(
       *Instance.get(), cache, VersionedPCMInstanceCacheCache.get(),
       Saver, BatchInput);

lib/DependencyScan/ScanDependencies.cpp

@@ -1109,7 +1109,7 @@ forEachBatchEntry(CompilerInstance &invocationInstance,
       newGlobalCache->configureForTriple(
           newInstance->getInvocation().getLangOptions().Target.str());
       auto newLocalCache = std::make_unique<ModuleDependenciesCache>(
-          *newGlobalCache, newInstance->getMainModule()->getNameStr());
+          *newGlobalCache);
       pInstance = newInstance.get();
       pCache = newLocalCache.get();
       subInstanceMap->insert(
@@ -1253,8 +1253,7 @@ bool swift::dependencies::scanDependencies(CompilerInstance &instance) {
   if (opts.ReuseDependencyScannerCache)
     deserializeDependencyCache(instance, globalCache);
 
-  ModuleDependenciesCache cache(globalCache,
-                                instance.getMainModule()->getNameStr());
+  ModuleDependenciesCache cache(globalCache);
 
   // Execute scan
   auto dependenciesOrErr = performModuleScan(instance, cache);
@@ -1289,8 +1288,7 @@ bool swift::dependencies::prescanDependencies(CompilerInstance &instance) {
   GlobalModuleDependenciesCache singleUseGlobalCache;
   singleUseGlobalCache.configureForTriple(instance.getInvocation()
                                                   .getLangOptions().Target.str());
-  ModuleDependenciesCache cache(singleUseGlobalCache,
-                                instance.getMainModule()->getNameStr());
+  ModuleDependenciesCache cache(singleUseGlobalCache);
   if (out.has_error() || EC) {
     Context.Diags.diagnose(SourceLoc(), diag::error_opening_output, path,
                            EC.message());
@@ -1321,8 +1319,7 @@ bool swift::dependencies::batchScanDependencies(
   GlobalModuleDependenciesCache singleUseGlobalCache;
   singleUseGlobalCache.configureForTriple(instance.getInvocation()
                                                   .getLangOptions().Target.str());
-  ModuleDependenciesCache cache(singleUseGlobalCache,
-                                instance.getMainModule()->getNameStr());
+  ModuleDependenciesCache cache(singleUseGlobalCache);
   (void)instance.getMainModule();
   llvm::BumpPtrAllocator alloc;
   llvm::StringSaver saver(alloc);
@@ -1357,8 +1354,7 @@ bool swift::dependencies::batchPrescanDependencies(
   GlobalModuleDependenciesCache singleUseGlobalCache;
   singleUseGlobalCache.configureForTriple(instance.getInvocation()
                                                   .getLangOptions().Target.str());
-  ModuleDependenciesCache cache(singleUseGlobalCache,
-                                instance.getMainModule()->getNameStr());
+  ModuleDependenciesCache cache(singleUseGlobalCache);
   (void)instance.getMainModule();
   llvm::BumpPtrAllocator alloc;
   llvm::StringSaver saver(alloc);

lib/Frontend/Frontend.cpp

@@ -21,6 +21,7 @@
 #include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/FileSystem.h"
 #include "swift/AST/Module.h"
+#include "swift/AST/ModuleDependencies.h"
 #include "swift/AST/TypeCheckRequests.h"
 #include "swift/Basic/FileTypes.h"
 #include "swift/Basic/SourceManager.h"

lib/Sema/SourceLoader.cpp

@@ -20,6 +20,7 @@
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/Module.h"
+#include "swift/AST/ModuleDependencies.h"
 #include "swift/AST/SourceFile.h"
 #include "swift/Parse/PersistentParserState.h"
 #include "swift/Basic/SourceManager.h"
@@ -149,3 +150,11 @@ void SourceLoader::loadExtensions(NominalTypeDecl *nominal,
   // Type-checking the source automatically loads all extensions; there's
   // nothing to do here.
 }
+
+Optional<ModuleDependencies>
+SourceLoader::getModuleDependencies(StringRef moduleName,
+                                    ModuleDependenciesCache &cache,
+                                    InterfaceSubContextDelegate &delegate) {
+  // FIXME: Implement?
+  return None;
+}