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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/FS.cpp:29
+PreambleFileStatusCache::lookup(llvm::StringRef File) const {
+  auto I = StatCache.find(File);
+  if (I != StatCache.end())
----------------
sammccall wrote:
> ioeric wrote:
> > 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.
> Indeed if you have the following sequencing:
> 
>  - one FS writes to the cache from one thread (or several but strictly sequenced)
>  - sequence point (no writes after this point, no reads before)
>  - several FSs read from the cache
> 
> then no lock is required, either for writer or reader.
> The API doesn't particularly suggest this, so please explicitly document the threading model in the header.
> (An API that would suggest it is to e.g. make the producing FS the top-level object, give it a &&-qualified method to retrieve the cache, and give the cache methods to retrieve the consuming FSes. But I think that's awkward here with the VFS interface)
(Stole some wording here. Thanks!)


================
Comment at: clangd/FS.cpp:53
+      // likely to be cached in the underlying file system anyway.
+      if (auto S = File->get()->status())
+        StatCache.update(getUnderlyingFS(), std::move(*S));
----------------
sammccall wrote:
> do you want to check if the file is already in the stat cache first?
This would always invalidate status in cache when file is reopened or restated in case file status has changed during preamble build. Added a comment.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52419