qchateau updated this revision to Diff 313215.
qchateau marked 3 inline comments as done.
qchateau added a comment.
The log is back, I renamed the CMake option and the command line option and
reduced the number of preprocessor directives.
I chose not to modify the files for the `gn` build system, I'd rather not break
it.
I did not use the CMke option as the default value for the command line option:
IMO this CMake option is only useful if you encounter build problems related to
malloc_trim, so malloc_trim must not be part of the code when you disable it
through CMake.
If this all makes sense and I did not make a mistake in this update, you can
land this. Let me know if there are other small details you'd like me to change.
Email: [email protected]
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93452/new/
https://reviews.llvm.org/D93452
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/Features.inc.in
clang-tools-extra/clangd/tool/ClangdMain.cpp
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===================================================================
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -50,6 +50,10 @@
#include <unistd.h>
#endif
+#ifdef __GLIBC__
+#include <malloc.h>
+#endif
+
namespace clang {
namespace clangd {
@@ -497,6 +501,29 @@
init(ClangdServer::Options().CollectMainFileRefs),
};
+#if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
+opt<bool> EnableMallocTrim{
+ "malloc-trim",
+ cat(Misc),
+ desc("Release memory periodically via malloc_trim(3)."),
+ init(true),
+};
+
+std::function<void()> getMemoryCleanupFunction() {
+ if (!EnableMallocTrim)
+ return nullptr;
+ // Leave a few MB at the top of the heap: it is insignificant
+ // and will most likely be needed by the main thread
+ constexpr size_t MallocTrimPad = 20'000'000;
+ return []() {
+ if (malloc_trim(MallocTrimPad))
+ vlog("Released memory via malloc_trim");
+ };
+}
+#else
+std::function<void()> getMemoryCleanupFunction() { return nullptr; }
+#endif
+
#if CLANGD_ENABLE_REMOTE
opt<std::string> RemoteIndexAddress{
"remote-index-address",
@@ -797,6 +824,7 @@
Opts.BuildRecoveryAST = RecoveryAST;
Opts.PreserveRecoveryASTType = RecoveryASTType;
Opts.FoldingRanges = FoldingRanges;
+ Opts.MemoryCleanup = getMemoryCleanupFunction();
Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
Opts.CodeComplete.Limit = LimitResults;
Index: clang-tools-extra/clangd/Features.inc.in
===================================================================
--- clang-tools-extra/clangd/Features.inc.in
+++ clang-tools-extra/clangd/Features.inc.in
@@ -1,2 +1,3 @@
#define CLANGD_BUILD_XPC @CLANGD_BUILD_XPC@
#define CLANGD_ENABLE_REMOTE @CLANGD_ENABLE_REMOTE@
+#define CLANGD_MALLOC_TRIM @CLANGD_MALLOC_TRIM@
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -48,6 +48,9 @@
llvm::Optional<Path> CompileCommandsDir;
/// The offset-encoding to use, or None to negotiate it over LSP.
llvm::Optional<OffsetEncoding> Encoding;
+ /// If set, periodically called to release memory.
+ /// Consider malloc_trim(3)
+ std::function<void()> MemoryCleanup = nullptr;
/// Per-feature options. Generally ClangdServer lets these vary
/// per-request, but LSP allows limited/no customizations.
@@ -184,10 +187,18 @@
/// profiling hasn't happened recently.
void maybeExportMemoryProfile();
+ /// 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;
+
/// Since initialization of CDBs and ClangdServer is done lazily, the
/// following context captures the one used while creating ClangdLSPServer and
/// passes it to above mentioned object instances to make sure they share the
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -178,6 +178,7 @@
} else if (auto Handler = Notifications.lookup(Method)) {
Handler(std::move(Params));
Server.maybeExportMemoryProfile();
+ Server.maybeCleanupMemory();
} else {
log("unhandled notification {0}", Method);
}
@@ -453,6 +454,7 @@
void ClangdLSPServer::notify(llvm::StringRef Method, llvm::json::Value Params) {
log("--> {0}", Method);
+ maybeCleanupMemory();
std::lock_guard<std::mutex> Lock(TranspWriter);
Transp.notify(Method, std::move(Params));
}
@@ -1295,6 +1297,27 @@
NextProfileTime = Now + ProfileInterval;
}
+void ClangdLSPServer::maybeCleanupMemory() {
+ // Memory cleanup is probably expensive, throttle it
+ static constexpr auto MemoryCleanupInterval = std::chrono::minutes(1);
+
+ if (!Opts.MemoryCleanup)
+ 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();
+}
+
// FIXME: This function needs to be properly tested.
void ClangdLSPServer::onChangeConfiguration(
const DidChangeConfigurationParams &Params) {
@@ -1501,8 +1524,9 @@
MsgHandler->bind("textDocument/foldingRange", &ClangdLSPServer::onFoldingRange);
// clang-format on
- // Delay first profile until we've finished warming up.
- NextProfileTime = std::chrono::steady_clock::now() + std::chrono::minutes(1);
+ // 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() {
@@ -1615,6 +1639,10 @@
void ClangdLSPServer::onBackgroundIndexProgress(
const BackgroundQueue::Stats &Stats) {
static const char ProgressToken[] = "backgroundIndexProgress";
+
+ // The background index did some work, maybe we need to cleanup
+ maybeCleanupMemory();
+
std::lock_guard<std::mutex> Lock(BackgroundIndexProgressMutex);
auto NotifyProgress = [this](const BackgroundQueue::Stats &Stats) {
Index: clang-tools-extra/clangd/CMakeLists.txt
===================================================================
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -14,9 +14,12 @@
unset(CLANGD_BUILD_XPC_DEFAULT)
endif ()
+option(CLANGD_MALLOC_TRIM "Call malloc_trim(3) periodically in Clangd. (only takes effect when using glibc)" ON)
+
llvm_canonicalize_cmake_booleans(
CLANGD_BUILD_XPC
CLANGD_ENABLE_REMOTE
+ CLANGD_MALLOC_TRIM
LLVM_ENABLE_ZLIB
)
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
