This revision was automatically updated to reflect the committed changes.
Closed by commit rL343576: [clangd] Cache FS stat() calls when building
preamble. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D52419
Files:
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE343576: [clangd] Cache FS stat() calls when building
preamble. (authored by ioeric, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D52419?vs=167914=167915#toc
Repository:
rCTE
ioeric updated this revision to Diff 167914.
ioeric added a comment.
- Rebase
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52419
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/CodeComplete.cpp
ioeric updated this revision to Diff 167913.
ioeric added a comment.
- add comment for StatCache in PreambleData
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52419
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Sorry for dropping this.
Comment at: clangd/ClangdUnit.h:58
IncludeStructure Includes;
+ std::unique_ptr StatCache;
};
This deserves a comment,
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
ioeric updated this revision to Diff 167370.
ioeric marked 2 inline comments as done.
ioeric added a comment.
- Address review comments
- address review comments
- address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52419
Files:
clangd/CMakeLists.txt
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
ioeric added inline comments.
Comment at: clangd/ClangdUnit.cpp:339
+llvm::ErrorOr status(const Twine ) override {
+ auto I = StatCache.find(Path.str());
+ return (I != StatCache.end()) ? I->getValue() : FS->status(Path);
ilya-biryukov wrote:
>
ioeric updated this revision to Diff 167124.
ioeric marked 7 inline comments as done.
ioeric added a comment.
- address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52419
Files:
clangd/CMakeLists.txt
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:339
+llvm::ErrorOr status(const Twine ) override {
+ auto I = StatCache.find(Path.str());
+ return (I != StatCache.end()) ? I->getValue() : FS->status(Path);
ioeric wrote:
>
sammccall added a comment.
Generally LG. A few things I'm unsure about.
Comment at: clangd/CodeComplete.cpp:1028
+ IntrusiveRefCntPtr VFS = Input.VFS;
+ if (Input.PreambleStatCache)
ioeric wrote:
> ilya-biryukov wrote:
> > It feels like we could apply this
ioeric added a comment.
Thanks for the reviews!
Comment at: clangd/ClangdUnit.cpp:155
+auto S = FS->status(Path);
+if (S)
+ cacheStatus(*S);
sammccall wrote:
> why are you only caching success?
I had a comment in `openFileForRead`:
```
// Only
ioeric updated this revision to Diff 166951.
ioeric marked 10 inline comments as done.
ioeric added a comment.
Herald added a subscriber: mgorny.
- Address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52419
Files:
clangd/CMakeLists.txt
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:
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.cpp:119
+/// Collect and cache all file status from the underlying file system.
+class CollectStatCacheVFS : public vfs::FileSystem {
ioeric wrote:
> Would it make sense to add a
ioeric added inline comments.
Comment at: clangd/ClangdUnit.cpp:119
+/// Collect and cache all file status from the underlying file system.
+class CollectStatCacheVFS : public vfs::FileSystem {
Would it make sense to add a `clang::vfs::ProxyFS` that proxies
ioeric updated this revision to Diff 17.
ioeric added a comment.
- update comment
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52419
Files:
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/CodeComplete.cpp
clangd/CodeComplete.h
ioeric created this revision.
ioeric added reviewers: sammccall, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.
ioeric updated this revision to Diff 17.
ioeric added a comment.
- update comment
The file stats can be reused when preamble is reused
19 matches
Mail list logo
