[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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:
clang-tools-extra/trunk/clangd/CMakeLists.txt
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.h
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/clangd/FS.cpp
clang-tools-extra/trunk/clangd/FS.h
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
clang-tools-extra/trunk/unittests/clangd/FSTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
Index: clang-tools-extra/trunk/clangd/FS.cpp
===
--- clang-tools-extra/trunk/clangd/FS.cpp
+++ clang-tools-extra/trunk/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 , vfs::Status S) {
+ 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
+PreambleFileStatusCache::lookup(llvm::StringRef File) const {
+ auto I = StatCache.find(File);
+ if (I != StatCache.end())
+return I->getValue();
+ return llvm::None;
+}
+
+IntrusiveRefCntPtr PreambleFileStatusCache::getProducingFS(
+IntrusiveRefCntPtr FS) {
+ // This invalidates old status in cache if files are re-`open()`ed or
+ // re-`stat()`ed in case file status has changed during preamble build.
+ class CollectFS : public vfs::ProxyFileSystem {
+ public:
+CollectFS(IntrusiveRefCntPtr FS,
+ PreambleFileStatusCache )
+: ProxyFileSystem(std::move(FS)), StatCache(StatCache) {}
+
+llvm::ErrorOr>
+openFileForRead(const Twine ) 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 status(const Twine ) override {
+ auto S = getUnderlyingFS().status(Path);
+ if (S)
+StatCache.update(getUnderlyingFS(), *S);
+ return S;
+}
+
+ private:
+PreambleFileStatusCache
+ };
+ return IntrusiveRefCntPtr(new CollectFS(std::move(FS), *this));
+}
+
+IntrusiveRefCntPtr PreambleFileStatusCache::getConsumingFS(
+IntrusiveRefCntPtr FS) const {
+ class CacheVFS : public vfs::ProxyFileSystem {
+ public:
+CacheVFS(IntrusiveRefCntPtr FS,
+ const PreambleFileStatusCache )
+: ProxyFileSystem(std::move(FS)), StatCache(StatCache) {}
+
+llvm::ErrorOr status(const Twine ) override {
+ if (auto S = StatCache.lookup(Path.str()))
+return *S;
+ return getUnderlyingFS().status(Path);
+}
+
+ private:
+const PreambleFileStatusCache
+ };
+ return IntrusiveRefCntPtr(new CacheVFS(std::move(FS), *this));
+}
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/trunk/clangd/FS.h
===
--- clang-tools-extra/trunk/clangd/FS.h
+++ clang-tools-extra/trunk/clangd/FS.h
@@ -0,0 +1,65 @@
+//===--- 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"
+
+namespace clang {
+namespace clangd {
+
+/// Records status
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 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/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,71 @@
Field(::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 )
+: CountStats(CountStats) {}
+
+IntrusiveRefCntPtr getFileSystem() override {
+ class ListenStatVFS : public vfs::ProxyFileSystem {
+ public:
+ListenStatVFS(IntrusiveRefCntPtr FS,
+ llvm::StringMap )
+: ProxyFileSystem(std::move(FS)), CountStats(CountStats) {}
+
+llvm::ErrorOr>
+openFileForRead(const Twine ) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::openFileForRead(Path);
+}
+llvm::ErrorOr status(const Twine ) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::status(Path);
+}
+
+ private:
+llvm::StringMap
+ };
+
+ return IntrusiveRefCntPtr(
+ new ListenStatVFS(buildTestFS(Files), CountStats));
+}
+
+// If relative paths are used, they are resolved with testPath().
+llvm::StringMap Files;
+llvm::StringMap
+ };
+
+ llvm::StringMap 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(::Name, "TestSym")));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -23,6 +23,7 @@
llvm::StringMap const ) {
IntrusiveRefCntPtr MemFS(
new vfs::InMemoryFileSystem);
+ MemFS->setCurrentWorkingDirectory(testRoot());
for (auto : Files) {
StringRef File = FileAndContents.first();
MemFS->addFile(
Index: unittests/clangd/FSTests.cpp
===
--- unittests/clangd/FSTests.cpp
+++ 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 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);
+
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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
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 const ) {
IntrusiveRefCntPtr MemFS(
new vfs::InMemoryFileSystem);
+ MemFS->setCurrentWorkingDirectory(testRoot());
for (auto : 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 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(::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 )
+: CountStats(CountStats) {}
+
+IntrusiveRefCntPtr getFileSystem() override {
+ class ListenStatVFS : public vfs::ProxyFileSystem {
+ public:
+ListenStatVFS(IntrusiveRefCntPtr FS,
+ llvm::StringMap )
+: ProxyFileSystem(std::move(FS)), CountStats(CountStats) {}
+
+llvm::ErrorOr>
+openFileForRead(const Twine ) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::openFileForRead(Path);
+}
+llvm::ErrorOr status(const Twine ) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::status(Path);
+}
+
+ private:
+llvm::StringMap
+ };
+
+ return IntrusiveRefCntPtr(
+ new ListenStatVFS(buildTestFS(Files), CountStats));
+}
+
+// If relative paths are used, they are resolved with testPath().
+llvm::StringMap Files;
+llvm::StringMap
+ };
+
+ llvm::StringMap 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(::Name, "TestSym")));
+}
+
} // namespace
}
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/FS.cpp
clangd/FS.h
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/Merge.cpp
clangd/index/dex/Dex.cpp
clangd/tool/ClangdMain.cpp
unittests/clangd/CMakeLists.txt
unittests/clangd/ClangdTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/DexTests.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 const ) {
IntrusiveRefCntPtr MemFS(
new vfs::InMemoryFileSystem);
+ MemFS->setCurrentWorkingDirectory(testRoot());
for (auto : 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 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/DexTests.cpp
===
--- unittests/clangd/DexTests.cpp
+++ unittests/clangd/DexTests.cpp
@@ -523,6 +523,17 @@
EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1"));
}
+TEST(DexTest, WildcardScope) {
+ auto I =
+ Dex::build(generateSymbols({"a::y1", "a::b::y2", "c::y3"}), URISchemes);
+ FuzzyFindRequest Req;
+ Req.Query = "y";
+ Req.Scopes = {"a::"};
+ Req.AnyScope = true;
+ EXPECT_THAT(match(*I, Req),
+ UnorderedElementsAre("a::y1", "a::b::y2", "c::y3"));
+}
+
TEST(DexTest, IgnoreCases) {
auto I = Dex::build(generateSymbols({"ns::ABC", "ns::abc"}), URISchemes);
FuzzyFindRequest Req;
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -2093,6 +2093,57 @@
Has("bar.h\"", CompletionItemKind::File)));
}
+TEST(CompletionTest, NoAllScopesCompletionWhenQualified) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(
+ R"cpp(
+void f() { na::Clangd^ }
+ )cpp",
+ {cls("na::ClangdA"), cls("nx::ClangdX"), cls("Clangd3")}, Opts);
+ EXPECT_THAT(Results.Completions,
+ UnorderedElementsAre(
+ AllOf(Qualifier(""), Scope("na::"), Named("ClangdA";
+}
+
+TEST(CompletionTest, AllScopesCompletion) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes = true;
+
+ auto Results = completions(
+ R"cpp(
+namespace na {
+void f() { Clangd^ }
+}
+ )cpp",
+ {cls("nx::Clangd1"), cls("ny::Clangd2"), cls("Clangd3"),
+ cls("na::nb::Clangd4")},
+ Opts);
+ EXPECT_THAT(
+ Results.Completions,
+ UnorderedElementsAre(AllOf(Qualifier("nx::"), Named("Clangd1")),
+ AllOf(Qualifier("ny::"), Named("Clangd2")),
+ AllOf(Qualifier(""), Scope(""), Named("Clangd3")),
+ AllOf(Qualifier("nb::"), Named("Clangd4";
+}
+
+TEST(CompletionTest, NoQualifierIfShadowed) {
+ clangd::CodeCompleteOptions Opts = {};
+ Opts.AllScopes
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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, even if it's a bit repetitive. ```// Cache of FS operations performed when building the preamble. // When reusing a preamble, this cache can be consumed to save IO.``` 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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 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.
> Indeed if you have the following sequencing:
>
> - one FS writes to the cache from one thread (or several but strictly
> sequenced)
> - sequence point (no writes after this point, no reads before)
> - several FSs read from the cache
>
> then no lock is required, either for writer or reader.
> The API doesn't particularly suggest this, so please explicitly document the
> threading model in the header.
> (An API that would suggest it is to e.g. make the producing FS the top-level
> object, give it a &&-qualified method to retrieve the cache, and give the
> cache methods to retrieve the consuming FSes. But I think that's awkward here
> with the VFS interface)
(Stole some wording here. Thanks!)
Comment at: clangd/FS.cpp:53
+ // likely to be cached in the underlying file system anyway.
+ if (auto S = File->get()->status())
+StatCache.update(getUnderlyingFS(), std::move(*S));
sammccall wrote:
> do you want to check if the file is already in the stat cache first?
This would always invalidate status in cache when file is reopened or restated
in case file status has changed during preamble build. Added a comment.
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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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
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 const ) {
IntrusiveRefCntPtr MemFS(
new vfs::InMemoryFileSystem);
+ MemFS->setCurrentWorkingDirectory(testRoot());
for (auto : 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 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(::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 )
+: CountStats(CountStats) {}
+
+IntrusiveRefCntPtr getFileSystem() override {
+ class ListenStatVFS : public vfs::ProxyFileSystem {
+ public:
+ListenStatVFS(IntrusiveRefCntPtr FS,
+ llvm::StringMap )
+: ProxyFileSystem(std::move(FS)), CountStats(CountStats) {}
+
+llvm::ErrorOr>
+openFileForRead(const Twine ) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::openFileForRead(Path);
+}
+llvm::ErrorOr status(const Twine ) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::status(Path);
+}
+
+ private:
+llvm::StringMap
+ };
+
+ return IntrusiveRefCntPtr(
+ new ListenStatVFS(buildTestFS(Files), CountStats));
+}
+
+// If relative paths are used, they are resolved with testPath().
+llvm::StringMap Files;
+llvm::StringMap
+ };
+
+ llvm::StringMap 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"],
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 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.
Indeed if you have the following sequencing:
- one FS writes to the cache from one thread (or several but strictly
sequenced)
- sequence point (no writes after this point, no reads before)
- several FSs read from the cache
then no lock is required, either for writer or reader.
The API doesn't particularly suggest this, so please explicitly document the
threading model in the header.
(An API that would suggest it is to e.g. make the producing FS the top-level
object, give it a &&-qualified method to retrieve the cache, and give the cache
methods to retrieve the consuming FSes. But I think that's awkward here with
the VFS interface)
Comment at: clangd/FS.cpp:48
+return File;
+ if (auto S = File->get()->status())
+StatCache.update(getUnderlyingFS(), std::move(*S));
ioeric wrote:
> 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.
Ah, OK.
The implementation comment is good, but this is significantly different from
"caching stat calls" as described in the header files.
Maybe update the comment there: e.g. "Records status information for files
open()ed or stat()ed during preamble build, so we can avoid stat()s on the
underlying FS when reusing the preamble"
Comment at: clangd/FS.cpp:53
+ // likely to be cached in the underlying file system anyway.
+ if (auto S = File->get()->status())
+StatCache.update(getUnderlyingFS(), std::move(*S));
do you want to check if the file is already in the stat cache first?
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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 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 = 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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 const ) {
IntrusiveRefCntPtr MemFS(
new vfs::InMemoryFileSystem);
+ MemFS->setCurrentWorkingDirectory(testRoot());
for (auto : 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 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(::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 )
+: CountStats(CountStats) {}
+
+IntrusiveRefCntPtr getFileSystem() override {
+ class ListenStatVFS : public vfs::ProxyFileSystem {
+ public:
+ListenStatVFS(IntrusiveRefCntPtr FS,
+ llvm::StringMap )
+: ProxyFileSystem(std::move(FS)), CountStats(CountStats) {}
+
+llvm::ErrorOr>
+openFileForRead(const Twine ) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::openFileForRead(Path);
+}
+llvm::ErrorOr status(const Twine ) override {
+ ++CountStats[llvm::sys::path::filename(Path.str())];
+ return ProxyFileSystem::status(Path);
+}
+
+ private:
+llvm::StringMap
+ };
+
+ return IntrusiveRefCntPtr(
+ new ListenStatVFS(buildTestFS(Files), CountStats));
+}
+
+// If relative paths are used, they are resolved with testPath().
+llvm::StringMap Files;
+llvm::StringMap
+ };
+
+ llvm::StringMap 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,
+
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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:
> 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.
Comment at: clangd/CodeComplete.cpp:1028
+ IntrusiveRefCntPtr VFS = Input.VFS;
+ if (Input.PreambleStatCache)
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.
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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 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?)
Comment at: clangd/FS.cpp:1
+//===--- FS.cpp - File system related utils --*-
C++-*-===//
+//
Can we have unit tests for these?
It should be pretty straightforward, as you can inspect/seed the cache state
directly.
Comment at: clangd/FS.cpp:29
+PreambleFileStatusCache::lookup(llvm::StringRef File) const {
+ auto I = StatCache.find(File);
+ if (I != StatCache.end())
lock
Comment at: clangd/FS.cpp:48
+return File;
+ if (auto S = File->get()->status())
+StatCache.update(getUnderlyingFS(), std::move(*S));
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 :-)
Comment at: clangd/FS.h:20
+/// 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.
can you give a little more colour on *why* code completion tends to stat a
bunch of the same files over again?
(One might assume it's to validate the preamble, but it shouldn't be, because
we don't do that for other reasons)
Comment at: clangd/FS.h:21
+/// 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.
+///
whene -> when
Comment at: unittests/clangd/ClangdTests.cpp:966
+TEST(ClangdTests, PreambleVFSStatCache) {
+ class ListenStatsFSProvider : public FileSystemProvider {
it's not obvious what this test does (without the rest of the patch as context).
Maybe a high-level comment: `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)`
Comment at: unittests/clangd/ClangdTests.cpp:980
+llvm::ErrorOr status(const Twine ) override {
+ auto I =
+ CountStats.try_emplace(llvm::sys::path::filename(Path.str()), 1);
just ++CountStats[...]?
life is too short :-)
Comment at: unittests/clangd/ClangdTests.cpp:984
+I.first->second += 1;
+ return getUnderlyingFS().status(Path);
+}
or just `return ProxyFileSystem::status(Path);` which is *slightly* less coupled
Comment at: unittests/clangd/ClangdTests.cpp:1018
+
+ unsigned Before = CountStats["foo.h"];
+ auto Completions = cantFail(runCodeComplete(Server, SourcePath, Code.point(),
I think this would be
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 cache stats for files that exist because, unlike building preamble,
// we only care about existing files when reusing preamble.
```
Another reason is that we are using the file name in the Status as the cache
key.
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:
> 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.
Comment at: clangd/CodeComplete.cpp:1028
+ IntrusiveRefCntPtr VFS = Input.VFS;
+ if (Input.PreambleStatCache)
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?
Comment at: clangd/CodeComplete.h:217
+CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
+const InputsAndPreamble ,
IntrusiveRefCntPtr VFS,
sammccall wrote:
> 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.
I thought `InputsAndPreamble` was a pretty natural fit here, but I guess I
hadn't understand the abstractions well enough. Thanks for the explanation!
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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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(::Name, "baz")));
}
+TEST(ClangdTests, PreambleVFSStatCache) {
+ class ListenStatsFSProvider : public FileSystemProvider {
+ public:
+ListenStatsFSProvider(llvm::StringMap )
+: CountStats(CountStats) {}
+
+IntrusiveRefCntPtr getFileSystem() override {
+ class ListenStatVFS : public vfs::ProxyFileSystem {
+ public:
+ListenStatVFS(IntrusiveRefCntPtr FS,
+ llvm::StringMap )
+: ProxyFileSystem(std::move(FS)), CountStats(CountStats) {}
+
+llvm::ErrorOr status(const Twine ) 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
+ };
+
+ return IntrusiveRefCntPtr(
+ new ListenStatVFS(buildTestFS(Files), CountStats));
+}
+
+// If relative paths are used, they are resolved with testPath().
+llvm::StringMap Files;
+llvm::StringMap
+ };
+
+ llvm::StringMap 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(::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
+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 , vfs::Status S);
+ /// \p Path is a path stored in preamble.
+ llvm::Optional 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
+ getProducingFS(IntrusiveRefCntPtr FS);
+
+ /// Returns a VFS that uses the cache collected.
+ ///
+ /// Note that the returned VFS should not outlive the cache.
+ IntrusiveRefCntPtr
+ getConsumingFS(IntrusiveRefCntPtr FS) const;
+
+private:
+ llvm::StringMap 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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 ) 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 ) {
+ 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
+createVFSWithPreambleStatCache(IntrusiveRefCntPtr FS,
+ const PreambleFileStatusCache );
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 ,
IntrusiveRefCntPtr 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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 `clang::vfs::ProxyFS` that proxies calls to
> underlying FS by default and allows to override some methods?
This makes sense, we seem to have a recurring pattern of forwarding all methods
by default in this commit.
+1 to the suggestion
Comment at: clangd/ClangdUnit.cpp:128
+std::error_code ) override {
+return FS->dir_begin(Dir, EC);
+ }
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.
Comment at: clangd/ClangdUnit.cpp:149
+if (auto S = File->get()->status())
+ cacheStatus(std::move(*S));
+return File;
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`.
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);
We store absolute paths, but Path can be relative here.
Comment at: clangd/ClangdUnit.cpp:340
+ auto I = StatCache.find(Path.str());
+ return (I != StatCache.end()) ? I->getValue() : FS->status(Path);
+}
Maybe remove parens?
Comparisons associate properly with ternary op and it's common enough to not
cause any confusion with the reader.
Comment at: clangd/CodeComplete.cpp:1003
const tooling::CompileCommand
- PrecompiledPreamble const *Preamble;
+ const PrecompiledPreamble *Preamble;
+ const PreambleFileStatusCache *PreambleStatCache;
Maybe pass a single pointer to `PreambleData` here?
Comment at: clangd/CodeComplete.cpp:1028
+ IntrusiveRefCntPtr VFS = Input.VFS;
+ if (Input.PreambleStatCache)
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?
Comment at: clangd/CodeComplete.cpp:1627
Options,
- {FileName, Command, Preamble, Contents, Pos, std::move(VFS),
- std::move(PCHs)});
+ {FileName, Command, Preamble, /*PreambleStatCache=*/nullptr, Contents,
+ Pos, std::move(VFS), std::move(PCHs)});
`signatureHelp` could also benefit from the cached fs, maybe pass it too?
See the other comments about applying this caching one level higher (on
TUScheduler level) too.
Comment at: clangd/CodeComplete.h:217
+CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
+const InputsAndPreamble ,
IntrusiveRefCntPtr VFS,
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.
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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 calls to
underlying FS by default and allows to override some methods?
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
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,89 @@
Field(::Name, "baz")));
}
+TEST(ClangdTests, PreambleVFSStatCache) {
+ class ListenStatsFSProvider : public FileSystemProvider {
+ public:
+ListenStatsFSProvider(llvm::StringMap )
+: CountStats(CountStats) {}
+
+IntrusiveRefCntPtr getFileSystem() override {
+ class ListenStatVFS : public vfs::FileSystem {
+ public:
+ListenStatVFS(IntrusiveRefCntPtr FS,
+ llvm::StringMap )
+: FS(std::move(FS)), CountStats(CountStats) {}
+
+vfs::directory_iterator dir_begin(const Twine ,
+ std::error_code ) override {
+ return FS->dir_begin(Dir, EC);
+}
+std::error_code setCurrentWorkingDirectory(const Twine ) override {
+ return FS->setCurrentWorkingDirectory(Path);
+}
+llvm::ErrorOr getCurrentWorkingDirectory() const override {
+ return FS->getCurrentWorkingDirectory();
+}
+std::error_code
+getRealPath(const Twine ,
+SmallVectorImpl ) const override {
+ return FS->getRealPath(Path, Output);
+}
+
+llvm::ErrorOr>
+openFileForRead(const Twine ) override {
+ return FS->openFileForRead(Path);
+}
+
+llvm::ErrorOr status(const Twine ) override {
+ auto I =
+ CountStats.try_emplace(llvm::sys::path::filename(Path.str()), 1);
+ if (!I.second)
+I.first->second += 1;
+ return FS->status(Path);
+}
+
+ private:
+IntrusiveRefCntPtr FS;
+llvm::StringMap
+ };
+
+ return IntrusiveRefCntPtr(
+ new ListenStatVFS(buildTestFS(Files), CountStats));
+}
+
+// If relative paths are used, they are resolved with testPath().
+llvm::StringMap Files;
+llvm::StringMap
+ };
+
+ llvm::StringMap 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(::Name, "TestSym")));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -20,6 +20,7 @@
#include "Logger.h"
#include "Path.h"
#include "Protocol.h"
+#include "TUScheduler.h"
#include "index/Index.h"
#include "clang/Frontend/PrecompiledPreamble.h"
#include "clang/Sema/CodeCompleteConsumer.h"
@@ -212,11 +213,8 @@
/// the speculative result is used by code completion (e.g. speculation failed),
/// the speculative result is not consumed, and `SpecFuzzyFind` is only
/// destroyed when the async request finishes.
-CodeCompleteResult codeComplete(PathRef FileName,
-const tooling::CompileCommand ,
-PrecompiledPreamble const *Preamble,
-const IncludeStructure ,
-StringRef Contents, Position Pos,
+CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
+const InputsAndPreamble ,
IntrusiveRefCntPtr VFS,
std::shared_ptr PCHs,
CodeCompleteOptions Opts,
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -1000,7 +1000,8 @@
struct SemaCompleteInput {
PathRef FileName;
const tooling::CompileCommand
- PrecompiledPreamble const *Preamble;
+ const PrecompiledPreamble *Preamble;
+ const PreambleFileStatusCache *PreambleStatCache;
StringRef Contents;
Position
[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.
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 (e.g. code
completion). It's safe to assume that cached status is not outdated as we
assume preamble files to remain unchanged.
On real file system, this made code completion ~20% faster on a measured file
(with big preamble). The preamble build time doesn't change much.
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
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,89 @@
Field(::Name, "baz")));
}
+TEST(ClangdTests, PreambleVFSStatCache) {
+ class ListenStatsFSProvider : public FileSystemProvider {
+ public:
+ListenStatsFSProvider(llvm::StringMap )
+: CountStats(CountStats) {}
+
+IntrusiveRefCntPtr getFileSystem() override {
+ class ListenStatVFS : public vfs::FileSystem {
+ public:
+ListenStatVFS(IntrusiveRefCntPtr FS,
+ llvm::StringMap )
+: FS(std::move(FS)), CountStats(CountStats) {}
+
+vfs::directory_iterator dir_begin(const Twine ,
+ std::error_code ) override {
+ return FS->dir_begin(Dir, EC);
+}
+std::error_code setCurrentWorkingDirectory(const Twine ) override {
+ return FS->setCurrentWorkingDirectory(Path);
+}
+llvm::ErrorOr getCurrentWorkingDirectory() const override {
+ return FS->getCurrentWorkingDirectory();
+}
+std::error_code
+getRealPath(const Twine ,
+SmallVectorImpl ) const override {
+ return FS->getRealPath(Path, Output);
+}
+
+llvm::ErrorOr>
+openFileForRead(const Twine ) override {
+ return FS->openFileForRead(Path);
+}
+
+llvm::ErrorOr status(const Twine ) override {
+ auto I =
+ CountStats.try_emplace(llvm::sys::path::filename(Path.str()), 1);
+ if (!I.second)
+I.first->second += 1;
+ return FS->status(Path);
+}
+
+ private:
+IntrusiveRefCntPtr FS;
+llvm::StringMap
+ };
+
+ return IntrusiveRefCntPtr(
+ new ListenStatVFS(buildTestFS(Files), CountStats));
+}
+
+// If relative paths are used, they are resolved with testPath().
+llvm::StringMap Files;
+llvm::StringMap
+ };
+
+ llvm::StringMap 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(::Name, "TestSym")));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -20,6 +20,7 @@
#include "Logger.h"
#include "Path.h"
#include "Protocol.h"
+#include "TUScheduler.h"
#include "index/Index.h"
#include "clang/Frontend/PrecompiledPreamble.h"
#include "clang/Sema/CodeCompleteConsumer.h"
@@ -212,11 +213,8 @@
/// the speculative result is used by code completion (e.g. speculation failed),
/// the speculative result is not consumed, and `SpecFuzzyFind` is only
/// destroyed when the async request finishes.
-CodeCompleteResult codeComplete(PathRef FileName,
-const tooling::CompileCommand ,
-PrecompiledPreamble const *Preamble,
-const IncludeStructure ,
-StringRef Contents, Position Pos,
+CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
+const InputsAndPreamble ,
IntrusiveRefCntPtr VFS,
std::shared_ptr PCHs,
