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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice win!

This is missing high-level documentation explaining what problem it's trying to solve and what the lifetime is.
I think pulling the factories out into a dedicated header would give you good space for that.



================
Comment at: clangd/ClangdUnit.cpp:128
+                                    std::error_code &EC) override {
+    return FS->dir_begin(Dir, EC);
+  }
----------------
ilya-biryukov wrote:
> 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.
The returned iterator can't be used to obtain file status (since my patch).


================
Comment at: clangd/ClangdUnit.cpp:149
+    if (auto S = File->get()->status())
+      cacheStatus(std::move(*S));
+    return File;
----------------
ilya-biryukov wrote:
> 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`.
I don't think I agree, what's the argument for caching both?

In the observed profile, stats are a problem but openFileForRead is not (in fact the files are never open).

This can't be a hard correctness requirement, because stat->open is racy anyway.


================
Comment at: clangd/ClangdUnit.cpp:155
+    auto S = FS->status(Path);
+    if (S)
+      cacheStatus(*S);
----------------
why are you only caching success?


================
Comment at: clangd/ClangdUnit.cpp:166
+    // Stores the latest status in cache as it can change in a preamble build.
+    StatCache.insert({PathStore, std::move(S)});
+  }
----------------
this isn't threadsafe.

Note that adding a lock here isn't enough, as you may have multiple FS instances referring to the same cache. The lock needs to live alongside the cache storage, so the latter should probably be a class.


================
Comment at: clangd/ClangdUnit.cpp:312
+    const PreambleFileStatusCache &StatCache) {
+  class CacheVFS : public vfs::FileSystem {
+  public:
----------------
outline this implementation next to the other one, and give them parallel names?

statCacheProducingFS/statCacheConsumingFS?

The design is hard to follow without this parallelism explicit.


================
Comment at: clangd/ClangdUnit.h:57
+IntrusiveRefCntPtr<vfs::FileSystem>
+createVFSWithPreambleStatCache(IntrusiveRefCntPtr<vfs::FileSystem> FS,
+                               const PreambleFileStatusCache &StatCache);
----------------
This doesn't belong in ClangdUnit.
(Potentially it could live in clang as a VFS util, but a `FS.h` or so here seems fine. Maybe merge with `FSProvider.h`)


================
Comment at: clangd/CodeComplete.h:217
+CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
+                                const InputsAndPreamble &Input,
                                 IntrusiveRefCntPtr<vfs::FileSystem> VFS,
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52419