diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -19,6 +19,7 @@ #include "support/Context.h" #include "support/MemoryTree.h" #include "support/Path.h" +#include "support/Threading.h" #include "clang/Tooling/Core/Replacement.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/StringSet.h" @@ -186,18 +187,12 @@ /// Runs profiling and exports memory usage metrics if tracing is enabled and /// profiling hasn't happened recently. void maybeExportMemoryProfile(); + PeriodicThrottler ShouldProfile; /// Run the MemoryCleanup callback if it's time. /// This method is thread safe. void maybeCleanupMemory(); - - /// Timepoint until which profiling is off. It is used to throttle profiling - /// requests. - std::chrono::steady_clock::time_point NextProfileTime; - - /// Next time we want to call the MemoryCleanup callback. - std::mutex NextMemoryCleanupTimeMutex; - std::chrono::steady_clock::time_point NextMemoryCleanupTime; + PeriodicThrottler ShouldCleanupMemory; /// Since initialization of CDBs and ClangdServer is done lazily, the /// following context captures the one used while creating ClangdLSPServer and diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -1285,13 +1285,7 @@ } void ClangdLSPServer::maybeExportMemoryProfile() { - if (!trace::enabled()) - return; - // Profiling might be expensive, so we throttle it to happen once every 5 - // minutes. - static constexpr auto ProfileInterval = std::chrono::minutes(5); - auto Now = std::chrono::steady_clock::now(); - if (Now < NextProfileTime) + if (!trace::enabled() || !ShouldProfile()) return; static constexpr trace::Metric MemoryUsage( @@ -1300,27 +1294,11 @@ MemoryTree MT; profile(MT); record(MT, "clangd_lsp_server", MemoryUsage); - NextProfileTime = Now + ProfileInterval; } void ClangdLSPServer::maybeCleanupMemory() { - // Memory cleanup is probably expensive, throttle it - static constexpr auto MemoryCleanupInterval = std::chrono::minutes(1); - - if (!Opts.MemoryCleanup) + if (!Opts.MemoryCleanup || !ShouldCleanupMemory()) return; - - // FIXME: this can probably be done without a mutex - // and the logic could be shared with maybeExportMemoryProfile - { - auto Now = std::chrono::steady_clock::now(); - std::lock_guard Lock(NextMemoryCleanupTimeMutex); - if (Now < NextMemoryCleanupTime) - return; - NextMemoryCleanupTime = Now + MemoryCleanupInterval; - } - - vlog("Calling memory cleanup callback"); Opts.MemoryCleanup(); } @@ -1481,10 +1459,15 @@ ClangdLSPServer::ClangdLSPServer(class Transport &Transp, const ThreadsafeFS &TFS, const ClangdLSPServer::Options &Opts) - : BackgroundContext(Context::current().clone()), Transp(Transp), + : ShouldProfile(/*Period=*/std::chrono::minutes(5), + /*Delay=*/std::chrono::minutes(1)), + ShouldCleanupMemory(/*Period=*/std::chrono::minutes(1), + /*Delay=*/std::chrono::minutes(1)), + BackgroundContext(Context::current().clone()), Transp(Transp), MsgHandler(new MessageHandler(*this)), TFS(TFS), SupportedSymbolKinds(defaultSymbolKinds()), SupportedCompletionItemKinds(defaultCompletionItemKinds()), Opts(Opts) { + // clang-format off MsgHandler->bind("initialize", &ClangdLSPServer::onInitialize); MsgHandler->bind("initialized", &ClangdLSPServer::onInitialized); @@ -1529,10 +1512,6 @@ if (Opts.FoldingRanges) MsgHandler->bind("textDocument/foldingRange", &ClangdLSPServer::onFoldingRange); // clang-format on - - // Delay first profile and memory cleanup until we've finished warming up. - NextMemoryCleanupTime = NextProfileTime = - std::chrono::steady_clock::now() + std::chrono::minutes(1); } ClangdLSPServer::~ClangdLSPServer() { diff --git a/clang-tools-extra/clangd/support/Threading.h b/clang-tools-extra/clangd/support/Threading.h --- a/clang-tools-extra/clangd/support/Threading.h +++ b/clang-tools-extra/clangd/support/Threading.h @@ -169,6 +169,35 @@ } }; +/// Used to guard an operation that should run at most every N seconds. +/// +/// Usage: +/// mutable PeriodicThrottler ShouldLog(std::chrono::seconds(1)); +/// void calledFrequently() { +/// if (ShouldLog()) +/// log("this is not spammy"); +/// } +/// +/// This class is threadsafe. If multiple threads are involved, then the guarded +/// operation still needs to be threadsafe! +class PeriodicThrottler { + using Stopwatch = std::chrono::steady_clock; + using Rep = Stopwatch::duration::rep; + + Rep Period; + std::atomic Next; + +public: + /// If Period is zero, the throttler will return true every time. + PeriodicThrottler(Stopwatch::duration Period, Stopwatch::duration Delay = {}) + : Period(Period.count()), + Next((Stopwatch::now() + Delay).time_since_epoch().count()) {} + + /// Returns whether the operation should run at this time. + /// operator() is safe to call concurrently. + bool operator()(); +}; + } // namespace clangd } // namespace clang #endif diff --git a/clang-tools-extra/clangd/support/Threading.cpp b/clang-tools-extra/clangd/support/Threading.cpp --- a/clang-tools-extra/clangd/support/Threading.cpp +++ b/clang-tools-extra/clangd/support/Threading.cpp @@ -116,5 +116,17 @@ CV.wait_until(Lock, D.time()); } +bool PeriodicThrottler::operator()() { + Rep Now = Stopwatch::now().time_since_epoch().count(); + Rep OldNext = Next.load(std::memory_order_acquire); + if (Now < OldNext) + return false; + // We're ready to run (but may be racing other threads). + // Work out the updated target time, and run if we successfully bump it. + Rep NewNext = Now + Period; + return Next.compare_exchange_strong(OldNext, NewNext, + std::memory_order_acq_rel); +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp b/clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp --- a/clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp +++ b/clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp @@ -10,6 +10,7 @@ #include "llvm/ADT/DenseMap.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include #include namespace clang { @@ -121,5 +122,25 @@ ASSERT_THAT(ValueA.load(), testing::AnyOf('A', 'B')); } +// It's hard to write a real test of this class, std::chrono is awkward to mock. +// But test some degenerate cases at least. +TEST(PeriodicThrottlerTest, Minimal) { + PeriodicThrottler Once(std::chrono::hours(24)); + EXPECT_TRUE(Once()); + EXPECT_FALSE(Once()); + EXPECT_FALSE(Once()); + + PeriodicThrottler Later(std::chrono::hours(24), + /*Delay=*/std::chrono::hours(24)); + EXPECT_FALSE(Later()); + EXPECT_FALSE(Later()); + EXPECT_FALSE(Later()); + + PeriodicThrottler Always(std::chrono::seconds(0)); + EXPECT_TRUE(Always()); + EXPECT_TRUE(Always()); + EXPECT_TRUE(Always()); +} + } // namespace clangd } // namespace clang