[clang][lex] Avoid `DirectoryLookup` copies

[jansvoboda11]

Summary

This patch fixes a performance regression introduced in D121685 that was caused by copying DirectoryLookup.

rdar://101206790

[vsapsai]

The change looks good to me. And based on the scope of D121685 it should be sufficient. Do you have any ideas for testing? If it would require significant testing infrastructure work, we can do that separately. But if you have some simple testing ideas in mind, that would be worth adding.

[ributzka]

We could invoke clang with the -stats option and compare the result against the expected number of stat calls.

[jansvoboda11]

In D136019#3860557, @ributzka wrote:

We could invoke clang with the -stats option and compare the result against the expected number of stat calls.

The -stats-file= option only tells you how many times Clang requested DirectoryEntry object from FileManager (i.e. calling FileManager::getDirectory(StringRef)). This function only touches the real file system once and caches information internally. Checking this doesn't really help us catch regressions like this one. It's a cheap operation and it's pretty common and reasonable for this number to change between commits.

What's actually expensive about this regression is calling HeaderSearch::loadSubdirectoryModuleMaps(DirectoryLookup &) multiple times, since Dir.haveSearchedAllModuleMaps() never changes (we only mutate the local DirectoryLookup copy). That function accesses the real filesystem via LLVM's VFS and that is actually the expensive operation:

llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
for (llvm::vfs::directory_iterator Dir = FS.dir_begin(DirNative, EC), DirEnd;
     Dir != DirEnd && !EC; Dir.increment(EC)) {
    //
}

It would be nice to have statistics for these as well, but we don't have any infrastructure for that ATM.

[jansvoboda11]

Add regression test

[ributzka]

clang/test/Modules/file-manager-lookup-count.m

73

The content of the stats file may change over time, so the test shouldn't limit it to just the listed entries. I would drop the first and last CHECK and change CHECK-NEXT to just CHECK.

[jansvoboda11]

Remove regression tests.

[ributzka]

LGTM

clang/lib/Lex/HeaderSearch.cpp

 
 Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName,
                                    SourceLocation ImportLoc,
                                    bool AllowExtraModuleMapSearch) {
   Module *Module = nullptr;
 
   // Look through the various header search paths to load any available module
   // maps, searching for a module map that describes this module.
-  for (DirectoryLookup Dir : search_dir_range()) {
+  for (DirectoryLookup &Dir : search_dir_range()) {
     if (Dir.isFramework()) {
       // Search for or infer a module map for a framework. Here we use
       // SearchName rather than ModuleName, to permit finding private modules
       // named FooPrivate in buggy frameworks named Foo.
       SmallString<128> FrameworkDirName;
       FrameworkDirName += Dir.getFrameworkDir()->getName();
       llvm::sys::path::append(FrameworkDirName, SearchName + ".framework");
       if (auto FrameworkDir =

clang/test/Modules/file-manager-lookup-count.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- include-0/sub/module.modulemap
+module sub_0 {}
+//--- include-1/sub/module.modulemap
+module sub_1 {}
+//--- include-2/sub/module.modulemap
+module sub_2 {}
+//--- include-3/sub/module.modulemap
+module sub_3 {}
+//--- include-4/sub/module.modulemap
+module sub_4 {}
+//--- include-5/sub/module.modulemap
+module sub_5 {}
+//--- include-6/sub/module.modulemap
+module sub_6 {}
+//--- include-7/sub/module.modulemap
+module sub_7 {}
+//--- include-8/sub/module.modulemap
+module sub_8 {}
+//--- include-9/sub/module.modulemap
+module sub_9 {}
+//--- include-A/sub/module.modulemap
+module A {}
+//--- include-B/sub/module.modulemap
+module B {}
+//--- include-C/sub/module.modulemap
+module C {}
+//--- include-D/sub/module.modulemap
+module D {}
+//--- include-E/sub/module.modulemap
+module E {}
+//--- include-F/sub/module.modulemap
+module F {}
+//--- include-G/sub/module.modulemap
+module G {}
+//--- include-H/sub/module.modulemap
+module H {}
+
+//--- tu.m
+@import A;
+@import B;
+@import C;
+@import D;
+@import E;
+@import F;
+@import G;
+@import H;
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:   -fsyntax-only %t/tu.m -stats-file=%t/stats \
+// RUN:   -I %t/include-0 \
+// RUN:   -I %t/include-1 \
+// RUN:   -I %t/include-2 \
+// RUN:   -I %t/include-3 \
+// RUN:   -I %t/include-4 \
+// RUN:   -I %t/include-5 \
+// RUN:   -I %t/include-6 \
+// RUN:   -I %t/include-7 \
+// RUN:   -I %t/include-8 \
+// RUN:   -I %t/include-9 \
+// RUN:   -I %t/include-A \
+// RUN:   -I %t/include-B \
+// RUN:   -I %t/include-C \
+// RUN:   -I %t/include-D \
+// RUN:   -I %t/include-E \
+// RUN:   -I %t/include-F \
+// RUN:   -I %t/include-G \
+// RUN:   -I %t/include-H
+
+// RUN: FileCheck --input-file %t/stats %s
+// CHECK:      {
+// CHECK-NEXT:   "file-search.NumDirCacheMisses": 156,
+// CHECK-NEXT:   "file-search.NumDirLookups": 427,
+// CHECK-NEXT:   "file-search.NumFileCacheMisses": 89,
+// CHECK-NEXT:   "file-search.NumFileLookups": 341
+// CHECK-NEXT: }