[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: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);
----------------
ioeric wrote:
> 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.
It's true that preamble stores and checks absolute paths, however clang can later access the same file using a relative path.
Calling makeAbsolute here shouldn't hurt and would obviously make it work in both cases.


================
Comment at: clangd/CodeComplete.cpp:1028
 
+  IntrusiveRefCntPtr<vfs::FileSystem> VFS = Input.VFS;
+  if (Input.PreambleStatCache)
----------------
sammccall wrote:
> ioeric wrote:
> > 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?
> > 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?
> 
> Right, this is a bit of a mess...
> Currently there are features that access the FS "directly". This includes CodeComplete which runs its own compile action, as well as things like switchSourceHeader or format. These invoke FSProvider.
> Then there are features that build an AST (which may need file accesses), and then may implicitly read files by querying the AST (the FS is attached to the FileManager or something). These use the FS obtained from the FSProvider **in the relevant addDocument call**.
> There's no fundamental reason we can't have both (access FS directly and use the AST) in which case we'll be using both filesystems together...
> 
> In the near term, making the TUScheduler-managed AST rebuild use the cache stored in the preamble seems like a nice win. (As you alluded to I don't think this change is in TUScheduler itself, rather buildAST?)
My idea would be to create and pass the updated **cached** FS into both `buildAST` and `codeComplete` higher up the call chain. This would allow localizing the code that creates caching VFSes in one file (TUScheduler.h or similar).

The advantage of receiving the patched-up FS in `buildAST` and `codeComplete` is that they don't have to care about this trick, making the implementation simpler.
The fewer functions across the codebase have to apply the trick the better, e.g. this would make sure we won't accidentally forget it to apply it when implementing a new feature.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52419