[PATCH] D93452: [clangd] Trim memory periodically

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks much closer to something we can land.
Bunch of nits :-)



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1306
+
+  {
+    auto Now = std::chrono::steady_clock::now();
----------------
can you leave a FIXME to do this without a lock?
I've been meaning to add a library for these "every N seconds" tasks that uses atomic instead.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:52
+    // Malloc trim minimal period
+    std::chrono::seconds MemoryCleanupPeriod = std::chrono::seconds(60);
 
----------------
I don't particularly think the interval needs to be set through options (memory profiling interval isn't), we can just hardcode this.

On the other hand, doing this at all should be optional, and I think we should enable it from ClangdMain - this is a weird thing for a library to be doing by default, and as it stands we'll be doing it in unit tests and stuff.

I think my favorite variant would be
```
/// If set, periodically called to release memory.
/// Consider llvm::sys::Process::ReleaseHeapMemory;
std::function<void()> MemoryCleanup = nullptr;
```


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:200
+  std::chrono::steady_clock::time_point NextMallocTrim;
+  std::chrono::seconds MallocTrimPeriod;
+
----------------
why do we store another copy of this instead of using the one in Opts?


================
Comment at: llvm/include/llvm/Config/config.h.cmake:139
 
+/* Define to 1 if you have the `mallinfo' function. */
+#cmakedefine HAVE_MALLOC_TRIM ${HAVE_MALLOC_TRIM}
----------------
nit: the `malloc_trim` function


================
Comment at: llvm/include/llvm/Support/Process.h:78
 
+  /// Attempts to free unused memory from the heap.
+  /// This function is basically a call to malloc_trim(3).
----------------
"free" is a bit vague here - return unused heap memory to the system?


================
Comment at: llvm/include/llvm/Support/Process.h:79
+  /// Attempts to free unused memory from the heap.
+  /// This function is basically a call to malloc_trim(3).
+  /// \param Pad Free space to leave at the top of the heap
----------------
nit: "On glibc malloc, this calls malloc_trim(3)"


================
Comment at: llvm/include/llvm/Support/Process.h:81
+  /// \param Pad Free space to leave at the top of the heap
+  /// \returns true if memory was actually released
+  static bool mallocTrim(size_t Pad);
----------------
I'm not sure this is terribly useful to report, and if someone wants to implement this for other OSes/allocators then it may not be available. Maybe just make this void?


================
Comment at: llvm/include/llvm/Support/Process.h:82
+  /// \returns true if memory was actually released
+  static bool mallocTrim(size_t Pad);
+
----------------
similarly in the name of portability, we could just pick a number like 10MB and hard-code it in the implementation, at least for now - maybe we don't end up needing this flexibility, and the semantics are probably peculiar to glibc


================
Comment at: llvm/include/llvm/Support/Process.h:82
+  /// \returns true if memory was actually released
+  static bool mallocTrim(size_t Pad);
+
----------------
sammccall wrote:
> similarly in the name of portability, we could just pick a number like 10MB and hard-code it in the implementation, at least for now - maybe we don't end up needing this flexibility, and the semantics are probably peculiar to glibc
I'd give this a more generic name like `ReleaseUnusedHeap`


================
Comment at: llvm/lib/Support/Unix/Process.inc:122
+#if defined(HAVE_MALLOC_TRIM)
+  return malloc_trim(Pad);
+#else
----------------
MaskRay wrote:
> If the user uses jemalloc/tcmalloc with glibc, malloc_trim may exist while the function may do nothing.
True. Unless I'm missing something, it won't be completely free - it'll still acquire glibc-malloc locks and walk over whatever arenas exist.

@MaskRay is there anything sensible to do about this? We can introduce a cmake variable or something you're supposed to set if you're using another allocator, but:
 - that seems fragile
 - fundamentally doesn't support switching allocators with LD_PRELOAD, I guess



================
Comment at: llvm/lib/Support/Unix/Process.inc:124
+#else
+#warning Cannot get malloc info on this platform
+  return false;
----------------
this is expected, we don't want a compiler warning here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93452/new/

https://reviews.llvm.org/D93452