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
clangd/ClangdUnit.h
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/FS.cpp
clangd/FS.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ClangdTests.cpp
unittests/clangd/FSTests.cpp
unittests/clangd/TestFS.cpp
Index: unittests/clangd/TestFS.cpp
===================================================================
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -23,6 +23,7 @@
llvm::StringMap<time_t> const &Timestamps) {
IntrusiveRefCntPtr<vfs::InMemoryFileSystem> MemFS(
new vfs::InMemoryFileSystem);
+ MemFS->setCurrentWorkingDirectory(testRoot());
for (auto &FileAndContents : Files) {
StringRef File = FileAndContents.first();
MemFS->addFile(
Index: unittests/clangd/FSTests.cpp
===================================================================
--- /dev/null
+++ unittests/clangd/FSTests.cpp
@@ -0,0 +1,46 @@
+//===-- FSTests.cpp - File system related tests -----------------*- 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 "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FSTests, PreambleStatusCache) {
+ llvm::StringMap<std::string> Files;
+ Files["x"] = "";
+ Files["y"] = "";
+ auto FS = buildTestFS(Files);
+ FS->setCurrentWorkingDirectory(testRoot());
+
+ PreambleFileStatusCache StatCache;
+ auto ProduceFS = StatCache.getProducingFS(FS);
+ EXPECT_TRUE(ProduceFS->openFileForRead("x"));
+ EXPECT_TRUE(ProduceFS->status("y"));
+
+ EXPECT_TRUE(StatCache.lookup(testPath("x")).hasValue());
+ EXPECT_TRUE(StatCache.lookup(testPath("y")).hasValue());
+
+ vfs::Status S("fake", llvm::sys::fs::UniqueID(0, 0),
+ std::chrono::system_clock::now(), 0, 0, 1024,
+ llvm::sys::fs::file_type::regular_file, llvm::sys::fs::all_all);
+ StatCache.update(*FS, S);
+ auto ConsumeFS = StatCache.getConsumingFS(FS);
+ auto Cached = ConsumeFS->status(testPath("fake"));
+ EXPECT_TRUE(Cached);
+ EXPECT_EQ(Cached->getName(), S.getName());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/ClangdTests.cpp
===================================================================
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,71 @@
Field(&CodeCompletion::Name, "baz")));
}
+// Check that running code completion doesn't stat() a bunch of files from the
+// preamble again. (They should be using the preamble's stat-cache)
+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<std::unique_ptr<vfs::File>>
+ openFileForRead(const Twine &Path) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::openFileForRead(Path);
+ }
+ llvm::ErrorOr<vfs::Status> status(const Twine &Path) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::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());
+
+ EXPECT_EQ(CountStats["foo.h"], 1u);
+ auto Completions = cantFail(runCodeComplete(Server, SourcePath, Code.point(),
+ clangd::CodeCompleteOptions()))
+ .Completions;
+ EXPECT_EQ(CountStats["foo.h"], 1u);
+ EXPECT_THAT(Completions,
+ ElementsAre(Field(&CodeCompletion::Name, "TestSym")));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===================================================================
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -21,6 +21,7 @@
FileDistanceTests.cpp
FileIndexTests.cpp
FindSymbolsTests.cpp
+ FSTests.cpp
FuzzyMatchTests.cpp
GlobalCompilationDatabaseTests.cpp
HeadersTests.cpp
Index: clangd/FS.h
===================================================================
--- /dev/null
+++ clangd/FS.h
@@ -0,0 +1,58 @@
+//===--- 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. For example, code
+/// completion can re-stat files when getting FileID for source locations stored
+/// in preamble (e.g. checking whether a location is in the main file).
+///
+/// The cache is keyed by absolute path of file name in cached status, as this
+/// is what preamble stores.
+///
+/// Note that the cache is only valid when reusing preamble.
+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,92 @@
+//===--- 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;
+ // Eagerly stat opened file, as the followup `status` call on the file
+ // doesn't necessarily go through this FS. This puts some extra work on
+ // preamble build, but it should be worth it as preamble can be reused
+ // many times (e.g. code completion) and the repeated status call is
+ // likely to be cached in the underlying file system anyway.
+ 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;
@@ -361,15 +365,19 @@
trace::Span Tracer("BuildAST");
SPAN_ATTACH(Tracer, "File", FileName);
- if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
+ auto VFS = Inputs.FS;
+ if (Preamble && Preamble->StatCache)
+ VFS = Preamble->StatCache->getConsumingFS(std::move(VFS));
+ if (VFS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
log("Couldn't set working directory when building the preamble.");
// We proceed anyway, our lit-tests rely on results for non-existing working
// dirs.
}
- return ParsedAST::build(
- llvm::make_unique<CompilerInvocation>(*Invocation), Preamble,
- llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents), PCHs, Inputs.FS);
+ return ParsedAST::build(llvm::make_unique<CompilerInvocation>(*Invocation),
+ Preamble,
+ llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents),
+ PCHs, std::move(VFS));
}
SourceLocation clangd::getBeginningOfIdentifier(ParsedAST &Unit,
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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
