Merge pull request #29950 from brentdax/less-crossover

Make cross-importing faster

Author: beccadax

include/swift/AST/Module.h

@@ -310,6 +310,18 @@ class ModuleDecl : public DeclContext, public TypeDecl {
   /// Add a file declaring a cross-import overlay.
   void addCrossImportOverlayFile(StringRef file);
 
+  /// If this method returns \c false, the module does not declare any
+  /// cross-import overlays.
+  ///
+  /// This is a quick check you can use to bail out of expensive logic early;
+  /// however, a \c true return doesn't guarantee that the module declares
+  /// cross-import overlays--it only means that it \em might declare some.
+  ///
+  /// (Specifically, this method checks if the module loader found any
+  /// swiftoverlay files, but does not load the files to see if they list any
+  /// overlay modules.)
+  bool mightDeclareCrossImportOverlays() const;
+
   /// Append to \p overlayNames the names of all modules that this module
   /// declares should be imported when \p bystanderName is imported.
   ///

lib/AST/Module.cpp

@@ -1564,6 +1564,10 @@ void ModuleDecl::addCrossImportOverlayFile(StringRef file) {
       .push_back(new (ctx) OverlayFile(ctx.AllocateCopy(file)));
 }
 
+bool ModuleDecl::mightDeclareCrossImportOverlays() const {
+  return !declaredCrossImports.empty();
+}
+
 void ModuleDecl::
 findDeclaredCrossImportOverlays(Identifier bystanderName,
                                 SmallVectorImpl<Identifier> &overlayNames,

lib/Sema/NameBinding.cpp

@@ -184,6 +184,13 @@ namespace {
     /// often enormous.
     SmallSetVector<ImportedModuleDesc, 64> crossImportableModules;
 
+    /// The subset of \c crossImportableModules which may declare cross-imports.
+    ///
+    /// This is a performance optimization. Since most modules do not register
+    /// any cross-imports, we can usually compare against this list, which is
+    /// much, much smaller than \c crossImportableModules.
+    SmallVector<ImportedModuleDesc, 16> crossImportDeclaringModules;
+
     /// The index of the next module in \c visibleModules that should be
     /// cross-imported.
     size_t nextModuleToCrossImport = 0;
@@ -231,13 +238,21 @@ namespace {
     ///   then adds them to \c unboundImports using source locations from \p I.
     void crossImport(ModuleDecl *M, UnboundImport &I);
 
+    /// Discovers any cross-imports between \p newImport and
+    /// \p oldImports and adds them to \c unboundImports, using source
+    /// locations from \p I.
+    void findCrossImportsInLists(UnboundImport &I,
+                                 ArrayRef<ImportedModuleDesc> declaring,
+                                 ArrayRef<ImportedModuleDesc> bystanding,
+                                 bool shouldDiagnoseRedundantCrossImports);
+
     /// Discovers any cross-imports between \p declaringImport and
     /// \p bystandingImport and adds them to \c unboundImports, using source
     /// locations from \p I.
     void findCrossImports(UnboundImport &I,
                           const ImportedModuleDesc &declaringImport,
                           const ImportedModuleDesc &bystandingImport,
-                          SmallVectorImpl<Identifier> &overlayNames);
+                          bool shouldDiagnoseRedundantCrossImports);
 
     /// Load a module referenced by an import statement.
     ///
@@ -853,55 +868,72 @@ void NameBinder::crossImport(ModuleDecl *M, UnboundImport &I) {
     // FIXME: Should we warn if M doesn't reexport underlyingModule?
     SF.addSeparatelyImportedOverlay(M, I.getUnderlyingModule().get());
 
-  // FIXME: Most of the comparisons we do here are probably unnecessary. We
-  // only need to findCrossImports() on pairs where at least one of the two
-  // modules declares cross-imports, and most modules don't. This is low-hanging
-  // performance fruit.
-
   auto newImports = crossImportableModules.getArrayRef()
-                        .slice(nextModuleToCrossImport);
+                        .drop_front(nextModuleToCrossImport);
+
+  if (newImports.empty())
+    // Nothing to do except crash when we read past the end of
+    // crossImportableModules in that assert at the bottom.
+    return;
+
   for (auto &newImport : newImports) {
     if (!canCrossImport(newImport))
       continue;
 
-    // Search imports up to, but not including or after, `newImport`.
+    // First we check if any of the imports of modules that have declared
+    // cross-imports have declared one with this module.
+    findCrossImportsInLists(I, crossImportDeclaringModules, {newImport},
+                            /*shouldDiagnoseRedundantCrossImports=*/false);
+
+    // If this module doesn't declare any cross-imports, we're done with this
+    // import.
+    if (!newImport.module.second->mightDeclareCrossImportOverlays())
+      continue;
+
+    // Fine, we need to do the slow-but-rare thing: check if this import
+    // declares a cross-import with any previous one.
     auto oldImports =
-        make_range(crossImportableModules.getArrayRef().data(), &newImport);
-    for (auto &oldImport : oldImports) {
-      if (!canCrossImport(oldImport))
+        // Slice from the start of crossImportableModules up to newImport.
+        llvm::makeArrayRef(crossImportableModules.getArrayRef().data(),
+                           &newImport);
+    findCrossImportsInLists(I, {newImport}, oldImports,
+                            /*shouldDiagnoseRedundantCrossImports=*/true);
+
+    // Add this to the list of imports everyone needs to check against.
+    crossImportDeclaringModules.push_back(newImport);
+  }
+
+  // Catch potential memory smashers
+  assert(newImports.data() ==
+             &crossImportableModules[nextModuleToCrossImport] &&
+         "findCrossImports() should never mutate visibleModules");
+
+  nextModuleToCrossImport = crossImportableModules.size();
+}
+
+void
+NameBinder::findCrossImportsInLists(UnboundImport &I,
+                                    ArrayRef<ImportedModuleDesc> declaring,
+                                    ArrayRef<ImportedModuleDesc> bystanding,
+                                    bool shouldDiagnoseRedundantCrossImports) {
+  for (auto &declaringImport : declaring) {
+    if (!canCrossImport(declaringImport))
+      continue;
+
+    for (auto &bystandingImport : bystanding) {
+      if (!canCrossImport(bystandingImport))
         continue;
 
-      SmallVector<Identifier, 2> newImportOverlays;
-      findCrossImports(I, newImport, oldImport, newImportOverlays);
-
-      SmallVector<Identifier, 2> oldImportOverlays;
-      findCrossImports(I, oldImport, newImport, oldImportOverlays);
-
-      // If both sides of the cross-import declare some of the same overlays,
-      // this will cause some strange name lookup behavior; let's warn about it.
-      for (auto name : newImportOverlays) {
-        if (llvm::is_contained(oldImportOverlays, name)) {
-          ctx.Diags.diagnose(I.importLoc, diag::cross_imported_by_both_modules,
-                             newImport.module.second->getName(),
-                             oldImport.module.second->getName(), name);
-        }
-      }
-
-      // If findCrossImports() ever changed the visibleModules list, we'd see
-      // memory smashers here.
-      assert(newImports.data() ==
-                 &crossImportableModules[nextModuleToCrossImport] &&
-             "findCrossImports() should never mutate visibleModules");
+      findCrossImports(I, declaringImport, bystandingImport,
+                       shouldDiagnoseRedundantCrossImports);
     }
   }
-
-  nextModuleToCrossImport = crossImportableModules.size();
 }
 
 void NameBinder::findCrossImports(UnboundImport &I,
                                   const ImportedModuleDesc &declaringImport,
                                   const ImportedModuleDesc &bystandingImport,
-                                  SmallVectorImpl<Identifier> &names) {
+                                  bool shouldDiagnoseRedundantCrossImports) {
   assert(&declaringImport != &bystandingImport);
 
   LLVM_DEBUG(
@@ -913,9 +945,17 @@ void NameBinder::findCrossImports(UnboundImport &I,
     ctx.Stats->getFrontendCounters().NumCrossImportsChecked++;
 
   // Find modules we need to import.
+  SmallVector<Identifier, 4> names;
   declaringImport.module.second->findDeclaredCrossImportOverlays(
       bystandingImport.module.second->getName(), names, I.importLoc);
 
+  // If we're diagnosing cases where we cross-import in both directions, get the
+  // inverse list. Otherwise, leave the list empty.
+  SmallVector<Identifier, 4> oppositeNames;
+  if (shouldDiagnoseRedundantCrossImports)
+    bystandingImport.module.second->findDeclaredCrossImportOverlays(
+        declaringImport.module.second->getName(), oppositeNames, I.importLoc);
+
   if (ctx.Stats && !names.empty())
     ctx.Stats->getFrontendCounters().NumCrossImportsFound++;
 
@@ -929,6 +969,11 @@ void NameBinder::findCrossImports(UnboundImport &I,
     unboundImports.emplace_back(declaringImport.module.second->getASTContext(),
                                 I, name, declaringImport, bystandingImport);
 
+    if (llvm::is_contained(oppositeNames, name))
+      ctx.Diags.diagnose(I.importLoc, diag::cross_imported_by_both_modules,
+                         declaringImport.module.second->getName(),
+                         bystandingImport.module.second->getName(), name);
+
     LLVM_DEBUG({
       auto &crossImportOptions = unboundImports.back().options;
       llvm::dbgs() << "  ";

test/CrossImport/Inputs/lib-templates/include/Submodule.swiftcrossimport/AlwaysImported.swiftoverlay

@@ -0,0 +1,5 @@
+%YAML 1.2
+---
+version: 1
+modules:
+  - name: _NeverImportedOverlay

test/CrossImport/Inputs/lib-templates/include/clang_library.h

@@ -1 +1,2 @@
+#include "clang_library_submodule.h"
 void fromClangLibrary();

test/CrossImport/Inputs/lib-templates/include/clang_library_submodule.h

@@ -0,0 +1 @@
+void fromClangLibrarySubmodule();

test/CrossImport/Inputs/lib-templates/include/module.modulemap

@@ -1 +1,6 @@
-module ClangLibrary { header "clang_library.h" }
+module ClangLibrary {
+  header "clang_library.h"
+  module Submodule {
+    header "clang_library_submodule.h"
+  }
+}

test/CrossImport/loading.swift

@@ -23,6 +23,7 @@
 // RUN: %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DTHIN_LIBRARY -emit-dependencies-path - | %FileCheck -check-prefixes=DEPS,DEPS-MOST -DMODULE=ThinLibrary -DTARGET=%module-target-triple %s
 // RUN: %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DFAT_LIBRARY -emit-dependencies-path - | %FileCheck -check-prefixes=DEPS,DEPS-MOST -DMODULE=FatLibrary -DTARGET=%module-target-triple %s
 // RUN: %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DCLANG_LIBRARY -emit-dependencies-path - | %FileCheck -check-prefixes=DEPS,DEPS-MOST -DMODULE=ClangLibrary -DTARGET=%module-target-triple %s
+// RUN: %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DCLANG_LIBRARY_SUBMODULE -emit-dependencies-path - | %FileCheck -check-prefixes=DEPS,DEPS-MOST -DMODULE=ClangLibrary -DTARGET=%module-target-triple %s
 // RUN: %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DSWIFT_FRAMEWORK -emit-dependencies-path - | %FileCheck -check-prefixes=DEPS,DEPS-MOST -DMODULE=SwiftFramework -DTARGET=%module-target-triple %s
 // RUN: %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DCLANG_FRAMEWORK -emit-dependencies-path - | %FileCheck -check-prefixes=DEPS,DEPS-MOST -DMODULE=ClangFramework -DTARGET=%module-target-triple %s
 // RUN: %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DOVERLAID_CLANG_FRAMEWORK -emit-dependencies-path - | %FileCheck -check-prefix=DEPS -DMODULE=OverlaidClangFramework -DTARGET=%module-target-triple %s
@@ -35,6 +36,7 @@
 // RUN: not %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DDONT_ALWAYS_IMPORT -DTHIN_LIBRARY 2>/dev/null
 // RUN: not %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DDONT_ALWAYS_IMPORT -DFAT_LIBRARY 2>/dev/null
 // RUN: not %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DDONT_ALWAYS_IMPORT -DCLANG_LIBRARY 2>/dev/null
+// RUN: not %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DDONT_ALWAYS_IMPORT -DCLANG_LIBRARY_SUBMODULE 2>/dev/null
 // RUN: not %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DDONT_ALWAYS_IMPORT -DSWIFT_FRAMEWORK 2>/dev/null
 // RUN: not %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DDONT_ALWAYS_IMPORT -DCLANG_FRAMEWORK 2>/dev/null
 // RUN: not %target-typecheck-verify-swift -enable-cross-import-overlays -I %t/include -I %t/lib/swift -F %t/Frameworks -DDONT_ALWAYS_IMPORT -DOVERLAID_CLANG_FRAMEWORK 2>/dev/null
@@ -108,6 +110,10 @@ import FatLibrary
 import ClangLibrary
 #endif
 
+#if CLANG_LIBRARY_SUBMODULE
+import ClangLibrary.Submodule
+#endif
+
 #if SWIFT_FRAMEWORK
 import SwiftFramework
 #endif