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