[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: thakis, ilya-biryukov, bkramer.
Herald added a subscriber: cfe-commits.

We don't immediately need the file to be open, and this code does not ensure
that it is closed. On at least some code paths (using headers from PCHs) the
file is never closed and we accumulate open file descriptors.

Prior to r347205, the file was not actually opened (despite shouldOpen=true)
if the FileEntry already existed. In the PCH case, the entry existed (due to
a previous call with shouldOpen=false) and so we got away with passing true.

r347205 "fixed" that behavior of FileManager: if the file was previously statted
but not opened, and then FileManager::get(shouldOpen=true) is called, then the
file is opened.
Subsequently we observe spurious "file not found" errors in projects that use
large PCHs on platforms with low open-file limits:

  https://bugs.chromium.org/p/chromium/issues/detail?id=924225

The fix here has non-obvious effects on the sequence of FS operations, due to
the subtle pattern of caches and side-effects in FileManager's usage in clang.
The main observed effects (from instrumenting FileSystem) are:

- It converts most open()+fstat() pairs for headers into a stat()+open() pair. This should be a performance loss: stat() is more expensive.
- It avoids open()s of some files where we turn out not to need the contents. This should be a performance win: stat() is cheaper, and open() usually requires an additional close().

At least in simple tests without PCHs, I observe the same total for
fstat+stat+open, e.g. fstat -51, stat +73, open -22.


Repository:
  rC Clang

https://reviews.llvm.org/D57150

Files:
  test/PCH/leakfiles


Index: test/PCH/leakfiles
===================================================================
--- /dev/null
+++ test/PCH/leakfiles
@@ -0,0 +1,29 @@
+// Test that compiling using a PCH doesn't leak file descriptors.
+// https://bugs.chromium.org/p/chromium/issues/detail?id=924225
+
+// This test requires bash loops and ulimit.
+// REQUIRES: shell
+// UNSUPPORTED: win32
+//
+// Set up source files. lib/lib.h includes lots of lib*.h files in that dir.
+// client.c includes lib/lib.h, and also the individual files directly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: mkdir lib
+// RUN: for i in {1..300}; do touch lib/lib$i.h; done
+// RUN: for i in {1..300}; do echo "#include \"lib$i.h\"" >> lib/lib.h; done
+// RUN: echo "#include \"lib/lib.h\"" > client.c
+// RUN: for i in {1..300}; do echo "#include \"lib/lib$i.h\"" >> client.c; done
+//
+// We want to verify that we don't hold all the files open at the same time.
+// This is important e.g. on mac, which has a low default FD limit.
+// RUN: ulimit -n 100
+//
+// Test without PCH.
+// RUN: %clang_cc1 -fsyntax-only -Ilib/ client.c
+//
+// Test with PCH.
+// RUN: %clang_cc1 -emit-pch -o pch -Ilib/ client.c
+// RUN: %clang_cc1 -include-pch pch -Ilib/ client.c -fsyntax-only


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57150.183290.patch
Type: text/x-patch
Size: 1258 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190124/6628626a/attachment-0001.bin>