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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:119
 
+/// Collect and cache all file status from the underlying file system.
+class CollectStatCacheVFS : public vfs::FileSystem {
----------------
ioeric wrote:
> Would it make sense to add a `clang::vfs::ProxyFS` that proxies calls to underlying FS by default and allows to override some methods?
This makes sense, we seem to have a recurring pattern of forwarding all methods by default in this commit. 
+1 to the suggestion


================
Comment at: clangd/ClangdUnit.cpp:128
+                                    std::error_code &EC) override {
+    return FS->dir_begin(Dir, EC);
+  }
----------------
One can obtain file stats from the returned iterator.
I believe this is exactly what will start happening after Sam's patch to list dirs when processing include dirs.

We should probably also wrap iterators that we return and cache stats from those.


================
Comment at: clangd/ClangdUnit.cpp:149
+    if (auto S = File->get()->status())
+      cacheStatus(std::move(*S));
+    return File;
----------------
If we cache the stats, we should probably cache the file contents too.
Not sure how much of an extra memory this will require.

Though maybe we could get away with returning the old stat, but new file contents. It would look as if the file was between calls to `stat()` and `openForRead`.


================
Comment at: clangd/ClangdUnit.cpp:339
+    llvm::ErrorOr<vfs::Status> status(const Twine &Path) override {
+      auto I = StatCache.find(Path.str());
+      return (I != StatCache.end()) ? I->getValue() : FS->status(Path);
----------------
We store absolute paths, but Path can be relative here.


================
Comment at: clangd/ClangdUnit.cpp:340
+      auto I = StatCache.find(Path.str());
+      return (I != StatCache.end()) ? I->getValue() : FS->status(Path);
+    }
----------------
Maybe remove parens? 
Comparisons associate properly with ternary op and it's common enough to not cause any confusion with the reader.


================
Comment at: clangd/CodeComplete.cpp:1003
   const tooling::CompileCommand &Command;
-  PrecompiledPreamble const *Preamble;
+  const PrecompiledPreamble *Preamble;
+  const PreambleFileStatusCache *PreambleStatCache;
----------------
Maybe pass a single pointer to `PreambleData` here?


================
Comment at: clangd/CodeComplete.cpp:1028
 
+  IntrusiveRefCntPtr<vfs::FileSystem> VFS = Input.VFS;
+  if (Input.PreambleStatCache)
----------------
It feels like we could apply this caching one level higher, on the TUScheduler level when creating the filesystem. It makes sense not only for code completion, but also for all other features, including rebuilds of ASTs that were washed out of the cache.
WDYT?


================
Comment at: clangd/CodeComplete.cpp:1627
       Options,
-      {FileName, Command, Preamble, Contents, Pos, std::move(VFS),
-       std::move(PCHs)});
+      {FileName, Command, Preamble, /*PreambleStatCache=*/nullptr, Contents,
+       Pos, std::move(VFS), std::move(PCHs)});
----------------
`signatureHelp` could also benefit from the cached fs, maybe pass it too?
See the other comments about applying this caching one level higher (on TUScheduler level) too.


================
Comment at: clangd/CodeComplete.h:217
+CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
+                                const InputsAndPreamble &Input,
                                 IntrusiveRefCntPtr<vfs::FileSystem> VFS,
----------------
We should not be depending `TUScheduler.h`, I suggest to pass `PreambleData`instead of `PrecompiledPreamble` instead.

Depending on `TUScheduler.h` our layering  in clangd: low-level implementation of features should not depend on higher-level building blocks like TUScheduler.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52419