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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
