sammccall created this revision.
sammccall added a reviewer: qchateau.
Herald added subscribers: usaxena95, kadircet, jfb, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D93726
Files:
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
Index: clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
+++ 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 @@
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
Index: clang-tools-extra/clangd/support/Threading.h
===================================================================
--- clang-tools-extra/clangd/support/Threading.h
+++ 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;
+ mutable 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
Index: clang-tools-extra/clangd/support/Threading.cpp
===================================================================
--- clang-tools-extra/clangd/support/Threading.cpp
+++ clang-tools-extra/clangd/support/Threading.cpp
@@ -116,5 +116,16 @@
CV.wait_until(Lock, D.time());
}
+bool PeriodicThrottler::operator()() {
+ Rep Now = Stopwatch::now().time_since_epoch().count();
+ Rep OldNext = Next.load();
+ 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);
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ 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
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ 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<std::mutex> 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() {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
