[clang-tools-extra] 808c285 - [clangd] Add tests that no-op changes are cheap
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 14 07:16:04 PDT 2020
Author: Sam McCall
Date: 2020-04-14T16:15:23+02:00
New Revision: 808c2855e11615a384df9667338aa52854a92fd5
URL: https://github.com/llvm/llvm-project/commit/808c2855e11615a384df9667338aa52854a92fd5
DIFF: https://github.com/llvm/llvm-project/commit/808c2855e11615a384df9667338aa52854a92fd5.diff
LOG: [clangd] Add tests that no-op changes are cheap
Summary:
We want to be sure they don't cause AST rebuilds or evict items from the cache.
D77847 is going to start sending spurious no-op changes (in case the preamble
was invalidated), this is cheap enough but we shouldn't regress that in future.
Reviewers: kadircet
Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D78048
Added:
Modified:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/ClangdTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 766cf7642155..f82bfa4f2682 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -703,9 +703,8 @@ void ClangdServer::semanticHighlights(
TUScheduler::InvalidateOnUpdate);
}
-std::vector<std::pair<Path, std::size_t>>
-ClangdServer::getUsedBytesPerFile() const {
- return WorkScheduler.getUsedBytesPerFile();
+llvm::StringMap<TUScheduler::FileStats> ClangdServer::fileStats() const {
+ return WorkScheduler.fileStats();
}
LLVM_NODISCARD bool
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index ae3da84c42c8..f6e7e0799d83 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -304,14 +304,14 @@ class ClangdServer {
void semanticHighlights(PathRef File,
Callback<std::vector<HighlightingToken>>);
- /// Returns estimated memory usage for each of the currently open files.
- /// The order of results is unspecified.
+ /// Returns estimated memory usage and other statistics for each of the
+ /// currently open files.
/// Overall memory usage of clangd may be significantly more than reported
/// here, as this metric does not account (at least) for:
/// - memory occupied by static and dynamic index,
/// - memory required for in-flight requests,
/// FIXME: those metrics might be useful too, we should add them.
- std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
+ llvm::StringMap<TUScheduler::FileStats> fileStats() const;
// Blocks the main thread until the server is idle. Only for use in tests.
// Returns false if the timeout expires.
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 382cfe38239f..26adcfd2b8f2 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -397,7 +397,7 @@ class ASTWorker {
/// getPossiblyStalePreamble() can be null even after this function returns.
void waitForFirstPreamble() const;
- std::size_t getUsedBytes() const;
+ TUScheduler::FileStats stats() const;
bool isASTCached() const;
private:
@@ -478,6 +478,8 @@ class ASTWorker {
// don't. When the old handle is destroyed, the old worker will stop reporting
// any results to the user.
bool CanPublishResults = true; /* GUARDED_BY(PublishMu) */
+ std::atomic<unsigned> ASTBuildCount = {0};
+ std::atomic<unsigned> PreambleBuildCount = {0};
SynchronizedTUStatus Status;
PreambleThread PreamblePeer;
@@ -661,11 +663,13 @@ void ASTWorker::runWithAST(
// FIXME: We might need to build a patched ast once preamble thread starts
// running async. Currently getPossiblyStalePreamble below will always
// return a compatible preamble as ASTWorker::update blocks.
- llvm::Optional<ParsedAST> NewAST =
- Invocation ? buildAST(FileName, std::move(Invocation),
- CompilerInvocationDiagConsumer.take(),
- FileInputs, getPossiblyStalePreamble())
- : None;
+ llvm::Optional<ParsedAST> NewAST;
+ if (Invocation) {
+ NewAST = buildAST(FileName, std::move(Invocation),
+ CompilerInvocationDiagConsumer.take(), FileInputs,
+ getPossiblyStalePreamble());
+ ++ASTBuildCount;
+ }
AST = NewAST ? std::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
}
// Make sure we put the AST back into the LRU cache.
@@ -732,6 +736,7 @@ void ASTWorker::updatePreamble(std::unique_ptr<CompilerInvocation> CI,
// running inside ASTWorker assumes internals won't change until it
// finishes.
if (Preamble != LatestPreamble) {
+ ++PreambleBuildCount;
// Cached AST is no longer valid.
IdleASTs.take(this);
RanASTCallback = false;
@@ -802,6 +807,7 @@ void ASTWorker::generateDiagnostics(
llvm::Optional<ParsedAST> NewAST = buildAST(
FileName, std::move(Invocation), CIDiags, Inputs, LatestPreamble);
auto RebuildDuration = DebouncePolicy::clock::now() - RebuildStartTime;
+ ++ASTBuildCount;
// Try to record the AST-build time, to inform future update debouncing.
// This is best-effort only: if the lock is held, don't bother.
std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock);
@@ -897,13 +903,16 @@ tooling::CompileCommand ASTWorker::getCurrentCompileCommand() const {
return FileInputs.CompileCommand;
}
-std::size_t ASTWorker::getUsedBytes() const {
+TUScheduler::FileStats ASTWorker::stats() const {
+ TUScheduler::FileStats Result;
+ Result.ASTBuilds = ASTBuildCount;
+ Result.PreambleBuilds = PreambleBuildCount;
// Note that we don't report the size of ASTs currently used for processing
// the in-flight requests. We used this information for debugging purposes
// only, so this should be fine.
- std::size_t Result = IdleASTs.getUsedBytes(this);
+ Result.UsedBytes = IdleASTs.getUsedBytes(this);
if (auto Preamble = getPossiblyStalePreamble())
- Result += Preamble->Preamble.getSize();
+ Result.UsedBytes += Preamble->Preamble.getSize();
return Result;
}
@@ -1345,13 +1354,11 @@ void TUScheduler::runWithPreamble(llvm::StringRef Name, PathRef File,
std::move(Task));
}
-std::vector<std::pair<Path, std::size_t>>
-TUScheduler::getUsedBytesPerFile() const {
- std::vector<std::pair<Path, std::size_t>> Result;
- Result.reserve(Files.size());
- for (auto &&PathAndFile : Files)
- Result.push_back({std::string(PathAndFile.first()),
- PathAndFile.second->Worker->getUsedBytes()});
+llvm::StringMap<TUScheduler::FileStats> TUScheduler::fileStats() const {
+ llvm::StringMap<TUScheduler::FileStats> Result;
+ for (const auto &PathAndFile : Files)
+ Result.try_emplace(PathAndFile.first(),
+ PathAndFile.second->Worker->stats());
return Result;
}
diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h
index b6b9e5ccae21..a7f5d2f493fb 100644
--- a/clang-tools-extra/clangd/TUScheduler.h
+++ b/clang-tools-extra/clangd/TUScheduler.h
@@ -197,9 +197,14 @@ class TUScheduler {
std::unique_ptr<ParsingCallbacks> ASTCallbacks = nullptr);
~TUScheduler();
- /// Returns estimated memory usage for each of the currently open files.
- /// The order of results is unspecified.
- std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
+ struct FileStats {
+ std::size_t UsedBytes = 0;
+ unsigned PreambleBuilds = 0;
+ unsigned ASTBuilds = 0;
+ };
+ /// Returns resources used for each of the currently open files.
+ /// Results are inherently racy as they measure activity of other threads.
+ llvm::StringMap<FileStats> fileStats() const;
/// Returns a list of files with ASTs currently stored in memory. This method
/// is not very reliable and is only used for test. E.g., the results will not
diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
index d15eba80ae29..8bb4cad52dba 100644
--- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -38,6 +38,7 @@ namespace clangd {
namespace {
+using ::testing::AllOf;
using ::testing::ElementsAre;
using ::testing::Field;
using ::testing::Gt;
@@ -496,7 +497,13 @@ int hello;
EXPECT_THAT(*Locations, ElementsAre(DeclAt(FooCpp, FooSource.range("two"))));
}
-TEST_F(ClangdVFSTest, MemoryUsage) {
+MATCHER_P4(Stats, Name, UsesMemory, PreambleBuilds, ASTBuilds, "") {
+ return arg.first() == Name && (arg.second.UsedBytes != 0) == UsesMemory &&
+ std::tie(arg.second.PreambleBuilds, ASTBuilds) ==
+ std::tie(PreambleBuilds, ASTBuilds);
+}
+
+TEST_F(ClangdVFSTest, FileStats) {
MockFSProvider FS;
ErrorCheckingCallbacks DiagConsumer;
MockCompilationDatabase CDB;
@@ -513,22 +520,23 @@ struct Something {
FS.Files[FooCpp] = "";
FS.Files[BarCpp] = "";
- EXPECT_THAT(Server.getUsedBytesPerFile(), IsEmpty());
+ EXPECT_THAT(Server.fileStats(), IsEmpty());
Server.addDocument(FooCpp, SourceContents);
Server.addDocument(BarCpp, SourceContents);
ASSERT_TRUE(Server.blockUntilIdleForTest());
- EXPECT_THAT(Server.getUsedBytesPerFile(),
- UnorderedElementsAre(Pair(FooCpp, Gt(0u)), Pair(BarCpp, Gt(0u))));
+ EXPECT_THAT(Server.fileStats(),
+ UnorderedElementsAre(Stats(FooCpp, true, 1, 1),
+ Stats(BarCpp, true, 1, 1)));
Server.removeDocument(FooCpp);
ASSERT_TRUE(Server.blockUntilIdleForTest());
- EXPECT_THAT(Server.getUsedBytesPerFile(), ElementsAre(Pair(BarCpp, Gt(0u))));
+ EXPECT_THAT(Server.fileStats(), ElementsAre(Stats(BarCpp, true, 1, 1)));
Server.removeDocument(BarCpp);
ASSERT_TRUE(Server.blockUntilIdleForTest());
- EXPECT_THAT(Server.getUsedBytesPerFile(), IsEmpty());
+ EXPECT_THAT(Server.fileStats(), IsEmpty());
}
TEST_F(ClangdVFSTest, InvalidCompileCommand) {
diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
index 961a798a53ae..7106e01a10e4 100644
--- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -608,6 +608,37 @@ TEST_F(TUSchedulerTests, EvictedAST) {
UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
}
+// We send "empty" changes to TUScheduler when we think some external event
+// *might* have invalidated current state (e.g. a header was edited).
+// Verify that this doesn't evict our cache entries.
+TEST_F(TUSchedulerTests, NoopChangesDontThrashCache) {
+ auto Opts = optsForTest();
+ Opts.RetentionPolicy.MaxRetainedASTs = 1;
+ TUScheduler S(CDB, Opts);
+
+ auto Foo = testPath("foo.cpp");
+ auto FooInputs = getInputs(Foo, "int x=1;");
+ auto Bar = testPath("bar.cpp");
+ auto BarInputs = getInputs(Bar, "int x=2;");
+
+ // After opening Foo then Bar, AST cache contains Bar.
+ S.update(Foo, FooInputs, WantDiagnostics::Auto);
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ S.update(Bar, BarInputs, WantDiagnostics::Auto);
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ ASSERT_THAT(S.getFilesWithCachedAST(), ElementsAre(Bar));
+
+ // Any number of no-op updates to Foo don't dislodge Bar from the cache.
+ S.update(Foo, FooInputs, WantDiagnostics::Auto);
+ S.update(Foo, FooInputs, WantDiagnostics::Auto);
+ S.update(Foo, FooInputs, WantDiagnostics::Auto);
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ ASSERT_THAT(S.getFilesWithCachedAST(), ElementsAre(Bar));
+ // In fact each file has been built only once.
+ ASSERT_EQ(S.fileStats().lookup(Foo).ASTBuilds, 1u);
+ ASSERT_EQ(S.fileStats().lookup(Bar).ASTBuilds, 1u);
+}
+
TEST_F(TUSchedulerTests, EmptyPreamble) {
TUScheduler S(CDB, optsForTest());
@@ -686,7 +717,7 @@ TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
Files[Header] = "int a;";
Timestamps[Header] = time_t(0);
- auto SourceContents = R"cpp(
+ std::string SourceContents = R"cpp(
#include "foo.h"
int b = a;
)cpp";
@@ -705,25 +736,32 @@ TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
// Test that subsequent updates with the same inputs do not cause rebuilds.
ASSERT_TRUE(DoUpdate(SourceContents));
+ ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 1u);
+ ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 1u);
ASSERT_FALSE(DoUpdate(SourceContents));
+ ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 1u);
+ ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 1u);
// Update to a header should cause a rebuild, though.
Timestamps[Header] = time_t(1);
ASSERT_TRUE(DoUpdate(SourceContents));
ASSERT_FALSE(DoUpdate(SourceContents));
+ ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 2u);
+ ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 2u);
// Update to the contents should cause a rebuild.
- auto OtherSourceContents = R"cpp(
- #include "foo.h"
- int c = d;
- )cpp";
- ASSERT_TRUE(DoUpdate(OtherSourceContents));
- ASSERT_FALSE(DoUpdate(OtherSourceContents));
+ SourceContents += "\nint c = b;";
+ ASSERT_TRUE(DoUpdate(SourceContents));
+ ASSERT_FALSE(DoUpdate(SourceContents));
+ ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 3u);
+ ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 2u);
// Update to the compile commands should also cause a rebuild.
CDB.ExtraClangFlags.push_back("-DSOMETHING");
- ASSERT_TRUE(DoUpdate(OtherSourceContents));
- ASSERT_FALSE(DoUpdate(OtherSourceContents));
+ ASSERT_TRUE(DoUpdate(SourceContents));
+ ASSERT_FALSE(DoUpdate(SourceContents));
+ ASSERT_EQ(S.fileStats().lookup(Source).ASTBuilds, 4u);
+ ASSERT_EQ(S.fileStats().lookup(Source).PreambleBuilds, 3u);
}
TEST_F(TUSchedulerTests, ForceRebuild) {
More information about the cfe-commits
mailing list