sammccall added inline comments.
================
Comment at: clangd/FS.cpp:29
+PreambleFileStatusCache::lookup(llvm::StringRef File) const {
+ auto I = StatCache.find(File);
+ if (I != StatCache.end())
----------------
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)
================
Comment at: clangd/FS.cpp:48
+ return File;
+ if (auto S = File->get()->status())
+ StatCache.update(getUnderlyingFS(), std::move(*S));
----------------
ioeric wrote:
> 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.
Ah, OK.
The implementation comment is good, but this is significantly different from
"caching stat calls" as described in the header files.
Maybe update the comment there: e.g. "Records status information for files
open()ed or stat()ed during preamble build, so we can avoid stat()s on the
underlying FS when reusing the preamble"
================
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));
----------------
do you want to check if the file is already in the stat cache first?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52419
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
