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
clangd/ClangdServer.cpp
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/FS.cpp
clangd/FS.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===================================================================
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,67 @@
Field(&CodeCompletion::Name, "baz")));
}
+TEST(ClangdTests, PreambleVFSStatCache) {
+ class ListenStatsFSProvider : public FileSystemProvider {
+ public:
+ ListenStatsFSProvider(llvm::StringMap<unsigned> &CountStats)
+ : CountStats(CountStats) {}
+
+ IntrusiveRefCntPtr<vfs::FileSystem> getFileSystem() override {
+ class ListenStatVFS : public vfs::ProxyFileSystem {
+ public:
+ ListenStatVFS(IntrusiveRefCntPtr<vfs::FileSystem> FS,
+ llvm::StringMap<unsigned> &CountStats)
+ : ProxyFileSystem(std::move(FS)), CountStats(CountStats) {}
+
+ llvm::ErrorOr<vfs::Status> status(const Twine &Path) override {
+ auto I =
+ CountStats.try_emplace(llvm::sys::path::filename(Path.str()), 1);
+ if (!I.second)
+ I.first->second += 1;
+ return getUnderlyingFS().status(Path);
+ }
+
+ private:
+ llvm::StringMap<unsigned> &CountStats;
+ };
+
+ return IntrusiveRefCntPtr<ListenStatVFS>(
+ new ListenStatVFS(buildTestFS(Files), CountStats));
+ }
+
+ // If relative paths are used, they are resolved with testPath().
+ llvm::StringMap<std::string> Files;
+ llvm::StringMap<unsigned> &CountStats;
+ };
+
+ llvm::StringMap<unsigned> CountStats;
+ ListenStatsFSProvider FS(CountStats);
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+ ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+ auto SourcePath = testPath("foo.cpp");
+ auto HeaderPath = testPath("foo.h");
+ FS.Files[HeaderPath] = "struct TestSym {};";
+ Annotations Code(R"cpp(
+ #include "foo.h"
+
+ int main() {
+ TestSy^
+ })cpp");
+
+ runAddDocument(Server, SourcePath, Code.code());
+
+ unsigned Before = CountStats["foo.h"];
+ auto Completions = cantFail(runCodeComplete(Server, SourcePath, Code.point(),
+ clangd::CodeCompleteOptions()))
+ .Completions;
+ EXPECT_EQ(CountStats["foo.h"], Before);
+ EXPECT_THAT(Completions,
+ ElementsAre(Field(&CodeCompletion::Name, "TestSym")));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/FS.h
===================================================================
--- /dev/null
+++ clangd/FS.h
@@ -0,0 +1,55 @@
+//===--- FS.h - File system related utils ------------------------*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FS_H
+
+#include "clang/Basic/VirtualFileSystem.h"
+#include "llvm/ADT/Optional.h"
+#include <mutex>
+namespace clang {
+namespace clangd {
+
+/// Caches `stat()` calls during preamble build, which can be used to avoid
+/// re-`stat`s header files when preamble is re-used (e.g. in code completion).
+/// Note that the cache is only valid whene reusing preamble.
+///
+/// The cache is keyed by absolute path of file name in cached status, as this
+/// is what preamble stores.
+class PreambleFileStatusCache {
+public:
+ void update(const vfs::FileSystem &FS, vfs::Status S);
+ /// \p Path is a path stored in preamble.
+ llvm::Optional<vfs::Status> lookup(llvm::StringRef Path) const;
+
+ /// Returns a VFS that collects file status.
+ /// Only cache stats for files that exist because
+ /// 1) we only care about existing files when reusing preamble, unlike
+ /// building preamble.
+ /// 2) we use the file name in the Status as the cache key.
+ ///
+ /// Note that the returned VFS should not outlive the cache.
+ IntrusiveRefCntPtr<vfs::FileSystem>
+ getProducingFS(IntrusiveRefCntPtr<vfs::FileSystem> FS);
+
+ /// Returns a VFS that uses the cache collected.
+ ///
+ /// Note that the returned VFS should not outlive the cache.
+ IntrusiveRefCntPtr<vfs::FileSystem>
+ getConsumingFS(IntrusiveRefCntPtr<vfs::FileSystem> FS) const;
+
+private:
+ llvm::StringMap<vfs::Status> StatCache;
+ mutable std::mutex Mutex;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_FS_H
Index: clangd/FS.cpp
===================================================================
--- /dev/null
+++ clangd/FS.cpp
@@ -0,0 +1,87 @@
+//===--- FS.cpp - File system related utils ----------------------*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "FS.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "llvm/ADT/None.h"
+
+namespace clang {
+namespace clangd {
+
+void PreambleFileStatusCache::update(const vfs::FileSystem &FS, vfs::Status S) {
+ std::lock_guard<std::mutex> Lock(Mutex);
+
+ SmallString<32> PathStore(S.getName());
+ if (auto Err = FS.makeAbsolute(PathStore))
+ return;
+ // Stores the latest status in cache as it can change in a preamble build.
+ StatCache.insert({PathStore, std::move(S)});
+}
+
+llvm::Optional<vfs::Status>
+PreambleFileStatusCache::lookup(llvm::StringRef File) const {
+ auto I = StatCache.find(File);
+ if (I != StatCache.end())
+ return I->getValue();
+ return llvm::None;
+}
+
+IntrusiveRefCntPtr<vfs::FileSystem> PreambleFileStatusCache::getProducingFS(
+ IntrusiveRefCntPtr<vfs::FileSystem> FS) {
+ class CollectFS : public vfs::ProxyFileSystem {
+ public:
+ CollectFS(IntrusiveRefCntPtr<vfs::FileSystem> FS,
+ PreambleFileStatusCache &StatCache)
+ : ProxyFileSystem(std::move(FS)), StatCache(StatCache) {}
+
+ llvm::ErrorOr<std::unique_ptr<vfs::File>>
+ openFileForRead(const Twine &Path) override {
+ auto File = getUnderlyingFS().openFileForRead(Path);
+ if (!File || !*File)
+ return File;
+ if (auto S = File->get()->status())
+ StatCache.update(getUnderlyingFS(), std::move(*S));
+ return File;
+ }
+
+ llvm::ErrorOr<vfs::Status> status(const Twine &Path) override {
+ auto S = getUnderlyingFS().status(Path);
+ if (S)
+ StatCache.update(getUnderlyingFS(), *S);
+ return S;
+ }
+
+ private:
+ PreambleFileStatusCache &StatCache;
+ };
+ return IntrusiveRefCntPtr<CollectFS>(new CollectFS(std::move(FS), *this));
+}
+
+IntrusiveRefCntPtr<vfs::FileSystem> PreambleFileStatusCache::getConsumingFS(
+ IntrusiveRefCntPtr<vfs::FileSystem> FS) const {
+ class CacheVFS : public vfs::ProxyFileSystem {
+ public:
+ CacheVFS(IntrusiveRefCntPtr<vfs::FileSystem> FS,
+ const PreambleFileStatusCache &StatCache)
+ : ProxyFileSystem(std::move(FS)), StatCache(StatCache) {}
+
+ llvm::ErrorOr<vfs::Status> status(const Twine &Path) override {
+ if (auto S = StatCache.lookup(Path.str()))
+ return *S;
+ return getUnderlyingFS().status(Path);
+ }
+
+ private:
+ const PreambleFileStatusCache &StatCache;
+ };
+ return IntrusiveRefCntPtr<CacheVFS>(new CacheVFS(std::move(FS), *this));
+}
+
+} // namespace clangd
+} // namespace clang
Index: clangd/CodeComplete.h
===================================================================
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -16,6 +16,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CODECOMPLETE_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CODECOMPLETE_H
+#include "ClangdUnit.h"
#include "Headers.h"
#include "Logger.h"
#include "Path.h"
@@ -214,8 +215,7 @@
/// destroyed when the async request finishes.
CodeCompleteResult codeComplete(PathRef FileName,
const tooling::CompileCommand &Command,
- PrecompiledPreamble const *Preamble,
- const IncludeStructure &PreambleInclusions,
+ const PreambleData *Preamble,
StringRef Contents, Position Pos,
IntrusiveRefCntPtr<vfs::FileSystem> VFS,
std::shared_ptr<PCHContainerOperations> PCHs,
@@ -225,8 +225,8 @@
/// Get signature help at a specified \p Pos in \p FileName.
SignatureHelp signatureHelp(PathRef FileName,
const tooling::CompileCommand &Command,
- PrecompiledPreamble const *Preamble,
- StringRef Contents, Position Pos,
+ const PreambleData *Preamble, StringRef Contents,
+ Position Pos,
IntrusiveRefCntPtr<vfs::FileSystem> VFS,
std::shared_ptr<PCHContainerOperations> PCHs,
const SymbolIndex *Index);
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -20,6 +20,7 @@
#include "CodeComplete.h"
#include "AST.h"
+#include "ClangdUnit.h"
#include "CodeCompletionStrings.h"
#include "Compiler.h"
#include "Diagnostics.h"
@@ -1000,7 +1001,7 @@
struct SemaCompleteInput {
PathRef FileName;
const tooling::CompileCommand &Command;
- PrecompiledPreamble const *Preamble;
+ const PreambleData *Preamble;
StringRef Contents;
Position Pos;
IntrusiveRefCntPtr<vfs::FileSystem> VFS;
@@ -1024,12 +1025,15 @@
// working dirs.
}
+ IntrusiveRefCntPtr<vfs::FileSystem> VFS = Input.VFS;
+ if (Input.Preamble && Input.Preamble->StatCache)
+ VFS = Input.Preamble->StatCache->getConsumingFS(std::move(VFS));
IgnoreDiagnostics DummyDiagsConsumer;
auto CI = createInvocationFromCommandLine(
ArgStrs,
CompilerInstance::createDiagnostics(new DiagnosticOptions,
&DummyDiagsConsumer, false),
- Input.VFS);
+ VFS);
if (!CI) {
elog("Couldn't create CompilerInvocation");
return false;
@@ -1068,8 +1072,10 @@
// NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise
// the remapped buffers do not get freed.
auto Clang = prepareCompilerInstance(
- std::move(CI), CompletingInPreamble ? nullptr : Input.Preamble,
- std::move(ContentsBuffer), std::move(Input.PCHs), std::move(Input.VFS),
+ std::move(CI),
+ (Input.Preamble && !CompletingInPreamble) ? &Input.Preamble->Preamble
+ : nullptr,
+ std::move(ContentsBuffer), std::move(Input.PCHs), std::move(VFS),
DummyDiagsConsumer);
Clang->getPreprocessorOpts().SingleFileParseMode = CompletingInPreamble;
Clang->setCodeCompletionConsumer(Consumer.release());
@@ -1588,19 +1594,20 @@
CodeCompleteResult
codeComplete(PathRef FileName, const tooling::CompileCommand &Command,
- PrecompiledPreamble const *Preamble,
- const IncludeStructure &PreambleInclusions, StringRef Contents,
- Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+ const PreambleData *Preamble, StringRef Contents, Position Pos,
+ IntrusiveRefCntPtr<vfs::FileSystem> VFS,
std::shared_ptr<PCHContainerOperations> PCHs,
CodeCompleteOptions Opts, SpeculativeFuzzyFind *SpecFuzzyFind) {
- return CodeCompleteFlow(FileName, PreambleInclusions, SpecFuzzyFind, Opts)
+ return CodeCompleteFlow(FileName,
+ Preamble ? Preamble->Includes : IncludeStructure(),
+ SpecFuzzyFind, Opts)
.run({FileName, Command, Preamble, Contents, Pos, VFS, PCHs});
}
SignatureHelp signatureHelp(PathRef FileName,
const tooling::CompileCommand &Command,
- PrecompiledPreamble const *Preamble,
- StringRef Contents, Position Pos,
+ const PreambleData *Preamble, StringRef Contents,
+ Position Pos,
IntrusiveRefCntPtr<vfs::FileSystem> VFS,
std::shared_ptr<PCHContainerOperations> PCHs,
const SymbolIndex *Index) {
Index: clangd/ClangdUnit.h
===================================================================
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -11,6 +11,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H
#include "Diagnostics.h"
+#include "FS.h"
#include "Function.h"
#include "Headers.h"
#include "Path.h"
@@ -45,14 +46,16 @@
// Stores Preamble and associated data.
struct PreambleData {
PreambleData(PrecompiledPreamble Preamble, std::vector<Diag> Diags,
- IncludeStructure Includes);
+ IncludeStructure Includes,
+ std::unique_ptr<PreambleFileStatusCache> StatCache);
tooling::CompileCommand CompileCommand;
PrecompiledPreamble Preamble;
std::vector<Diag> Diags;
// Processes like code completions and go-to-definitions will need #include
// information, and their compile action skips preamble range.
IncludeStructure Includes;
+ std::unique_ptr<PreambleFileStatusCache> StatCache;
};
/// Information required to run clang, e.g. to parse AST or do code completion.
Index: clangd/ClangdUnit.cpp
===================================================================
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -246,9 +246,10 @@
}
PreambleData::PreambleData(PrecompiledPreamble Preamble,
- std::vector<Diag> Diags, IncludeStructure Includes)
+ std::vector<Diag> Diags, IncludeStructure Includes,
+ std::unique_ptr<PreambleFileStatusCache> StatCache)
: Preamble(std::move(Preamble)), Diags(std::move(Diags)),
- Includes(std::move(Includes)) {}
+ Includes(std::move(Includes)), StatCache(std::move(StatCache)) {}
ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,
std::unique_ptr<CompilerInstance> Clang,
@@ -334,9 +335,12 @@
// We proceed anyway, our lit-tests rely on results for non-existing working
// dirs.
}
+
+ auto StatCache = llvm::make_unique<PreambleFileStatusCache>();
auto BuiltPreamble = PrecompiledPreamble::Build(
- CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine, Inputs.FS, PCHs,
- StoreInMemory, SerializedDeclsCollector);
+ CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine,
+ StatCache->getProducingFS(Inputs.FS), PCHs, StoreInMemory,
+ SerializedDeclsCollector);
// When building the AST for the main file, we do want the function
// bodies.
@@ -347,7 +351,7 @@
FileName);
return std::make_shared<PreambleData>(
std::move(*BuiltPreamble), PreambleDiagnostics.take(),
- SerializedDeclsCollector.takeIncludes());
+ SerializedDeclsCollector.takeIncludes(), std::move(StatCache));
} else {
elog("Could not build a preamble for file {0}", FileName);
return nullptr;
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -181,8 +181,6 @@
if (isCancelled())
return CB(llvm::make_error<CancelledError>());
- auto PreambleData = IP->Preamble;
-
llvm::Optional<SpeculativeFuzzyFind> SpecFuzzyFind;
if (CodeCompleteOpts.Index && CodeCompleteOpts.SpeculativeIndexRequest) {
SpecFuzzyFind.emplace();
@@ -195,10 +193,8 @@
// FIXME(ibiryukov): even if Preamble is non-null, we may want to check
// both the old and the new version in case only one of them matches.
CodeCompleteResult Result = clangd::codeComplete(
- File, IP->Command, PreambleData ? &PreambleData->Preamble : nullptr,
- PreambleData ? PreambleData->Includes : IncludeStructure(),
- IP->Contents, Pos, FS, PCHs, CodeCompleteOpts,
- SpecFuzzyFind ? SpecFuzzyFind.getPointer() : nullptr);
+ File, IP->Command, IP->Preamble, IP->Contents, Pos, FS, PCHs,
+ CodeCompleteOpts, SpecFuzzyFind ? SpecFuzzyFind.getPointer() : nullptr);
{
clang::clangd::trace::Span Tracer("Completion results callback");
CB(std::move(Result));
@@ -231,9 +227,8 @@
return CB(IP.takeError());
auto PreambleData = IP->Preamble;
- CB(clangd::signatureHelp(File, IP->Command,
- PreambleData ? &PreambleData->Preamble : nullptr,
- IP->Contents, Pos, FS, PCHs, Index));
+ CB(clangd::signatureHelp(File, IP->Command, PreambleData, IP->Contents, Pos,
+ FS, PCHs, Index));
};
// Unlike code completion, we wait for an up-to-date preamble here.
Index: clangd/CMakeLists.txt
===================================================================
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -21,6 +21,7 @@
DraftStore.cpp
FindSymbols.cpp
FileDistance.cpp
+ FS.cpp
FuzzyMatch.cpp
GlobalCompilationDatabase.cpp
Headers.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
