From 808c2855e11615a384df9667338aa52854a92fd5 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Mon, 13 Apr 2020 22:07:12 +0200 Subject: [PATCH] [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 --- clang-tools-extra/clangd/ClangdServer.cpp | 5 +- clang-tools-extra/clangd/ClangdServer.h | 6 +-- clang-tools-extra/clangd/TUScheduler.cpp | 39 ++++++++------- clang-tools-extra/clangd/TUScheduler.h | 11 +++-- clang-tools-extra/clangd/unittests/ClangdTests.cpp | 20 +++++--- .../clangd/unittests/TUSchedulerTests.cpp | 56 ++++++++++++++++++---- 6 files changed, 97 insertions(+), 40 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 766cf76..f82bfa4 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> -ClangdServer::getUsedBytesPerFile() const { - return WorkScheduler.getUsedBytesPerFile(); +llvm::StringMap 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 ae3da84..f6e7e07 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -304,14 +304,14 @@ public: void semanticHighlights(PathRef File, Callback>); - /// 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> getUsedBytesPerFile() const; + llvm::StringMap 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 382cfe3..26adcfd 100644 --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -397,7 +397,7 @@ public: /// 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 @@ private: // 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 ASTBuildCount = {0}; + std::atomic 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 NewAST = - Invocation ? buildAST(FileName, std::move(Invocation), - CompilerInvocationDiagConsumer.take(), - FileInputs, getPossiblyStalePreamble()) - : None; + llvm::Optional NewAST; + if (Invocation) { + NewAST = buildAST(FileName, std::move(Invocation), + CompilerInvocationDiagConsumer.take(), FileInputs, + getPossiblyStalePreamble()); + ++ASTBuildCount; + } AST = NewAST ? std::make_unique(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 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 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 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> -TUScheduler::getUsedBytesPerFile() const { - std::vector> Result; - Result.reserve(Files.size()); - for (auto &&PathAndFile : Files) - Result.push_back({std::string(PathAndFile.first()), - PathAndFile.second->Worker->getUsedBytes()}); +llvm::StringMap TUScheduler::fileStats() const { + llvm::StringMap 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 b6b9e5c..a7f5d2f 100644 --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -197,9 +197,14 @@ public: std::unique_ptr ASTCallbacks = nullptr); ~TUScheduler(); - /// Returns estimated memory usage for each of the currently open files. - /// The order of results is unspecified. - std::vector> 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() 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 d15eba8..8bb4cad 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 961a798..7106e01 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) { -- 2.7.4