[PATCH] D93452: [clangd] Trim memory periodically

[Quentin Chateau via Phabricator via llvm-commits]

qchateau planned changes to this revision.
qchateau marked 2 inline comments as done.
qchateau added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:52
+    // Malloc trim minimal period
+    std::chrono::seconds MemoryCleanupPeriod = std::chrono::seconds(60);
 
----------------
sammccall wrote:
> 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;
> ```
Sounds good, do you think [1] we should throttle the calls to `MemoryCleanup` or [2] throttle the calls to `malloc_trim` from within the callback ?

>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 like the idea of configuring the `MemoryCleanup` function from ClangdMain, but it's as easy to have [3] an int option that configures the interval as it is to have [4] a boolean one to just disable it.

I actually think both points are related:
- If we choose [2], then I'd go for [3] because it is more configurable and adds no code complexity.
- If we choose [1], then I agree we can go for [4] to avoid an extra option


================
Comment at: llvm/lib/Support/Unix/Process.inc:122
+#if defined(HAVE_MALLOC_TRIM)
+  return malloc_trim(Pad);
+#else
----------------
sammccall wrote:
> 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
> 
I'm not worried about LD_PRELOAD, worst case it adds a little bit of extra runtime cost (calling malloc_trim although it's useless because we preloaded tcmalloc).

But I agree there is an issue here: we detect which cleanup function to use depending on the headers (in cmake), but it should actually depend on what we link. Not sure if this can be done properly :/
For now we only want to cleanup on glibc malloc, but I'll investigate with tcmalloc and jemalloc, maybe they do not work so well either after all. Then the problem would be even worse: I'd be tempted to call a similar function for these libraries as well, and although the developer may have tcmalloc in its system include, it does not mean he is linking it -> undefined symbol

Overall this approach of calling `malloc_trim` is looking grim to me because it depends on what's linked.

Either we can continue in this way, or try to find another way around this issue:
- Encourage the developer to link another malloc implementation, but then it's only useful if the various distributions do it
- Continue to dig on why glibc malloc performs so bad with clangd and try to improve things in clangd code
- Something else ?



================
Comment at: llvm/lib/Support/Unix/Process.inc:124
+#else
+#warning Cannot get malloc info on this platform
+  return false;
----------------
sammccall wrote:
> this is expected, we don't want a compiler warning here
Same for windows I guess ? If I understand correctly your logic is "if it's not available that's because we don't need it: don't warn"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452