[PATCH] D93452: [clangd] Trim memory periodically

Sun Dec 20 05:16:45 PST 2020

sammccall added a comment.

Sorry to mess you around like this, but I think I might have jumped the gun in asking this to be abstracted away in `Process.h`. My reasoning is:

- Once we are injecting the cleanup function from ClangdMain, that's not actually a crazy place to put this kind of system-level code, behind #ifdefs
- The abstraction costs us something: we can't skip all the locking etc for memory cleanups if it's a no-op on our platform
- We're putting an abstraction around one implementation without any reason to believe other implementations need this treatment or can use a similar interface. This is likely to turn out to be a poor abstraction, and llvm/Support isn't the easiest place to iterate on it.

So I think my favorite option may be back to:

- Have ClangdLSPServer::Options take a nullable `std::function<void()>` to abstract the (optional) platform-specific operation
- Have ClangdMain contain platform #ifdefs and define+inject the wrapper that calls `malloc_trim`
- Optionally: have a clangd or cmake flag to disable this even if glibc was detected. This provides a workaround for other allocators if we need it.

Again, I'm sorry if it seems we're going in circles. I might put together a patch for this variant to compare...

In D93452#2464643 <https://reviews.llvm.org/D93452#2464643>, @njames93 wrote:

> Just attached a debugger and ran malloc_trim on my clangd instance which was sitting at 6.7GB, afterwards it was 1.3GB. Interestingly clangd held rock solid at 1.0GB recorded memory usage throughout.

Great to have confirmation of this, thanks!
Clangd's own memory-usage staying constant is expected (it's counting the live objects), as is real usage being higher (we count a subset, and we don't count malloc overhead/fragmentation).

In D93452#2464764 <https://reviews.llvm.org/D93452#2464764>, @qchateau wrote:

>> Interestingly clangd held rock solid at 1.0GB recorded memory usage throughout.
>
> What do you mean exactly ? Using the built in memory usage measurement ?

Yeah pretty sure this is the `$/memoryUsage` request or equivalent.

================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:52
+    // Malloc trim minimal period
+    std::chrono::seconds MemoryCleanupPeriod = std::chrono::seconds(60);

----------------
qchateau wrote:
> 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
> 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 think ClangdLSPServer should throttle the calls to the callback, because exactly what events trigger it internally is an implementation detail, and triggering periodically is a more useful API.

> 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.

If you configure the callback as a`std::function`, then it has a null state that naturally means "don't clean up memory", so you don't need the boolean. In this context I'm not sure having an option to configure is worthwhile.

================
Comment at: llvm/cmake/config-ix.cmake:235
 check_symbol_exists(mallinfo malloc.h HAVE_MALLINFO)
+check_symbol_exists(malloc_trim malloc.h HAVE_MALLOC_TRIM)
 check_symbol_exists(malloc_zone_statistics malloc/malloc.h
----------------
For clangd's purposes we really want to call this function if we're using glibc (and not finding it is bad!), and really don't want to call it otherwise (and don't want to call it even if we find it).
So we may just want to `#ifdef __GLIBC__` instead, especially if we do this in clangd instead of llvm/Support.

(In any case, this has been in glibc for a *long* time)

================
Comment at: llvm/lib/Support/Unix/Process.inc:122
+#if defined(HAVE_MALLOC_TRIM)
+  return malloc_trim(Pad);
+#else
----------------
qchateau wrote:
> 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 ?
> 
> Overall this approach of calling malloc_trim is looking grim to me because it depends on what's linked.

Let's not get lost in the weeds:
 - glibc malloc is by far the common case (on glibc platforms)
 - we've confirmed there's a major problem with this configuration, and it's essentially a well-known implementation deficiency specific to glibc malloc
 -`malloc_trim` solves this fairly simply
 - side-effects on uncommon configurations are likely to be minor
 - we don't know of any other common configurations that need a memory-trimming solution. (In the projects I've found that work around this, none mention similar problems on other platforms)

So this is a good-if-not-perfect solution, and I think we should ship it whether we can fix all the rough edges or not. It's probably going to be the single biggest improvement from clangd 11->12 for linux users.

================
Comment at: llvm/lib/Support/Unix/Process.inc:124
+#else
+#warning Cannot get malloc info on this platform
+  return false;
----------------
qchateau wrote:
> 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"
That, and also "warnings are annoying if you can't do anything about them". I think the `#warning` is there in the other functions because the `#if/#else` are expected-but-not-guaranteed to be exhaustive, because most platforms provide this functionality, which isn't the case here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452