[clang-tools-extra] 3dbe471 - [clangd] Use atomics instead of locks to track periodic memory trimming
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 22 13:32:31 PST 2020
Author: Sam McCall
Date: 2020-12-22T22:32:22+01:00
New Revision: 3dbe471a260392ec63dda8deb2709160afc56dde
URL: https://github.com/llvm/llvm-project/commit/3dbe471a260392ec63dda8deb2709160afc56dde
DIFF: https://github.com/llvm/llvm-project/commit/3dbe471a260392ec63dda8deb2709160afc56dde.diff
LOG: [clangd] Use atomics instead of locks to track periodic memory trimming
Instead of always locking/unlocking a contended mutex, we now do one atomic read
in the common case, and one read + one exchange if the timer has expried.
Also use this for memory profiling which has similar/compatible requirements.
Differential Revision: https://reviews.llvm.org/D93726
Added:
Modified:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/support/Threading.cpp
clang-tools-extra/clangd/support/Threading.h
clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 0c42f95fb594..c606ccae4fdc 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1285,13 +1285,7 @@ void ClangdLSPServer::publishDiagnostics(
}
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 @@ void ClangdLSPServer::maybeExportMemoryProfile() {
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<std::mutex> Lock(NextMemoryCleanupTimeMutex);
- if (Now < NextMemoryCleanupTime)
- return;
- NextMemoryCleanupTime = Now + MemoryCleanupInterval;
- }
-
- vlog("Calling memory cleanup callback");
Opts.MemoryCleanup();
}
@@ -1481,10 +1459,15 @@ void ClangdLSPServer::onAST(const ASTParams &Params,
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 @@ ClangdLSPServer::ClangdLSPServer(class Transport &Transp,
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/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index b5f9d2c9d766..a41bc5666af3 100644
--- 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 @@ class ClangdLSPServer : private ClangdServer::Callbacks {
/// 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/support/Threading.cpp b/clang-tools-extra/clangd/support/Threading.cpp
index 5f95888ae3e2..7f3bd62be306 100644
--- a/clang-tools-extra/clangd/support/Threading.cpp
+++ b/clang-tools-extra/clangd/support/Threading.cpp
@@ -116,5 +116,17 @@ void wait(std::unique_lock<std::mutex> &Lock, std::condition_variable &CV,
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/support/Threading.h b/clang-tools-extra/clangd/support/Threading.h
index 5155ac193fd1..da9e3b8ea8b6 100644
--- a/clang-tools-extra/clangd/support/Threading.h
+++ b/clang-tools-extra/clangd/support/Threading.h
@@ -169,6 +169,35 @@ template <typename Container> class Memoize {
}
};
+/// 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<Rep> 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/unittests/support/ThreadingTests.cpp b/clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
index e265ad2eabea..87002d3cfa86 100644
--- 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 <chrono>
#include <mutex>
namespace clang {
@@ -121,5 +122,25 @@ TEST_F(ThreadingTest, MemoizeDeterministic) {
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
More information about the cfe-commits
mailing list