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

[Eric Liu via Phabricator via cfe-commits]

ioeric 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);
----------------
ilya-biryukov wrote:
> 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.
It would "obviously work"... until you have symlinks and tricks to handle symlinks ;)


================
Comment at: clangd/CodeComplete.cpp:1028
 
+  IntrusiveRefCntPtr<vfs::FileSystem> VFS = Input.VFS;
+  if (Input.PreambleStatCache)
----------------
ilya-biryukov wrote:
> 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.
Added the cache support to `buildAST` as we thinks it's beneficial (haven't profiled this though).

Currently, the trick is applied in two places (`semaCodeComplete` in CodeComplete and `buildAST` in ClangdUnit), and I'm not sure how much this would grow in the near future. It's also unclear to me whether baking this into `TUScheduler` would be less work in the long run. I would suggest revisiting this when we have more evidence.


================
Comment at: clangd/FS.cpp:29
+PreambleFileStatusCache::lookup(llvm::StringRef File) const {
+  auto I = StatCache.find(File);
+  if (I != StatCache.end())
----------------
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.


================
Comment at: clangd/FS.cpp:48
+        return File;
+      if (auto S = File->get()->status())
+        StatCache.update(getUnderlyingFS(), std::move(*S));
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52419