[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/FS.cpp:29
+PreambleFileStatusCache::lookup(llvm::StringRef File) const {
+  auto I = StatCache.find(File);
+  if (I != StatCache.end())
----------------
ioeric wrote:
> sammccall wrote:
> > lock
> After a second thought, I'm wondering if the mutex is necessary, if the cache is only updated during preamble build in a single thread. The cache would also remain the same in preamble reuses.
Indeed if you have the following sequencing:

 - one FS writes to the cache from one thread (or several but strictly sequenced)
 - sequence point (no writes after this point, no reads before)
 - several FSs read from the cache

then no lock is required, either for writer or reader.
The API doesn't particularly suggest this, so please explicitly document the threading model in the header.
(An API that would suggest it is to e.g. make the producing FS the top-level object, give it a &&-qualified method to retrieve the cache, and give the cache methods to retrieve the consuming FSes. But I think that's awkward here with the VFS interface)


================
Comment at: clangd/FS.cpp:48
+        return File;
+      if (auto S = File->get()->status())
+        StatCache.update(getUnderlyingFS(), std::move(*S));
----------------
ioeric wrote:
> sammccall wrote:
> > I'm not sure I get this: AFAICT (at least on linux) the status is never available on a newly opened file, so this will always be a stat() call, so we're just doing the work eagerly and caching it for later. Isn't this just moving the work around?
> > 
> > I'm sure you've verified this is important somehow, but a comment explaining how would be much appreciated :-)
> Files opened via `openFileForRead()` wouldn't usually trigger `status()` calls on the wrap FS. And most header files are opened instead of `stat`ed.
> 
> Added comment explaining this.
Ah, OK.
The implementation comment is good, but this is significantly different from "caching stat calls" as described in the header files.

Maybe update the comment there: e.g. "Records status information for files open()ed or stat()ed during preamble build, so we can avoid stat()s on the underlying FS when reusing the preamble"


================
Comment at: clangd/FS.cpp:53
+      // likely to be cached in the underlying file system anyway.
+      if (auto S = File->get()->status())
+        StatCache.update(getUnderlyingFS(), std::move(*S));
----------------
do you want to check if the file is already in the stat cache first?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52419