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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the reviews!



================
Comment at: clangd/ClangdUnit.cpp:155
+    auto S = FS->status(Path);
+    if (S)
+      cacheStatus(*S);
----------------
sammccall wrote:
> why are you only caching success?
I had a comment in `openFileForRead`:
```
    // Only cache stats for files that exist because, unlike building preamble,
    // we only care about existing files when reusing preamble.
```

Another reason is that we are using the file name in the Status as the cache key.


================
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);
----------------
ilya-biryukov wrote:
> We store absolute paths, but Path can be relative here.
This is because preamble stores absolute paths. `Path` should be an path from preamble. Added documentation.


================
Comment at: clangd/CodeComplete.cpp:1028
 
+  IntrusiveRefCntPtr<vfs::FileSystem> VFS = Input.VFS;
+  if (Input.PreambleStatCache)
----------------
ilya-biryukov wrote:
> 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?
It sounds like a reasonable thing to do. I took a quick look, and it seemed that this would probably require some refactoring/redesign on TUScheduler - it doesn't know about the VFS?

I think it shouldn't be hard to do this case by case. For example, code completion and signature help will be covered in this patch. And it should be pretty easy to make AST rebuild use the cache as well?


================
Comment at: clangd/CodeComplete.h:217
+CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
+                                const InputsAndPreamble &Input,
                                 IntrusiveRefCntPtr<vfs::FileSystem> VFS,
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > 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.
> +1. We're maybe not doing the best job of header encapsulation here, but InputsAndPreamble is an implementation detail of ClangdServer.
I thought `InputsAndPreamble` was a pretty natural fit here, but I guess I hadn't understand the abstractions well enough. Thanks for the explanation!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52419