[PATCH] D93452: [clangd] Trim memory after buildINdex

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Wow, all abstractions truly are lies. Thanks for the investigation!

After doing some reading[1], my understanding is roughly:

- our pattern of allocations causes glibc malloc to retain a *much* larger heap than our outstanding allocations, growing without (or with large) bound
- malloc maintains several arenas and grows each as needed, and only implicitly shrinks them "from the top". A lives-forever object at the end of an arena will prevent it implicitly shrinking past that point.
- clangd has lives-almost-forever allocations, e.g. when background indexing that could be causing this. If we get unlucky and allocate it at a point when the arena happens to be really full, it doesn't matter if the arena spends most of its time mostly-empty.
- use of multiple threads means more arenas, and thus more fragmentation/waste.
- malloc_trim() is able to free more aggressively, looking "inside" the arenas [2]
- this affects glibc malloc only, which explains why we've only had this problem reported on linux and we haven't gotten reports from google-internal users (our production binaries use tcmalloc)

Some back-of-the-envelope numbers: I guess we grow until we have enough space to satisfy every allocation in every arena, so the wasted space/gaps is NumArenas times what you expect, which is 8 * NumCores, which can easily be 500 on a big workstation - if we expect (wild guess) 80% efficiency = 25% overhead for a single-threaded app, then this could yield 12500% overhead = 0.8% efficiency at the steady state, which is a 100x increase in memory usage. This sounds like a plausible explanation for the bug reports (but I'm not an expert and am guessing a lot).

[1] particularly: https://stackoverflow.com/questions/38644578/understanding-glibc-malloc-trimming
[2] https://man7.org/linux/man-pages/man3/malloc_trim.3.html - see notes regarding glibc 2.8

---

So where does this leave us, other than fairly unimpressed with glibc malloc?

Periodic malloc_trim seems like it solves a real problem, one I don't personally fully understand but we may never do - I feel convinced we should use it anyway. It probably has some overhead, who knows how much.

We need to work out how to guard it not just for glibc, but also for using glibc malloc rather than alternative like jemalloc, tcmalloc et al.
(We could *require* the use of an alternative malloc and not fix this, but that doesn't seem terribly practical to me)

It's process-wide (not per-thread, and certainly not per-program-module) so the fact that slapping it in FileSymbols::buildIndex solves the problem only proves that function is called periodically :-)
This is a system-level function, I think we should probably be calling it periodically from something top-level like ClangdMain or ClangdLSPServer.
(We could almost slip it in with ClangdLSPServer::maybeExportMemoryProfile(), except we don't want to link it to incoming notifications as memory can probably grow while silently background indexing)

I think we should pass a nonzero `pad` to malloc_trim of several MB. It only affects the main thread, but the main thread is constantly allocating small short-lived objects (e.g. JSON encoding/decoding) and cutting it to the bone only to have it sbrk again seems pointless.



================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:50
 
+void trimMemory() {
+  if (malloc_trim(0))
----------------
this function belongs in `llvm/Support/Process.h`, next to Process::GetMallocUsage().

This is full of OS-specific bits (`#ifdef HAVE_MALLINFO`) etc that probably guide how we should set this up.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:275
+  // (at the OS level): manually trim the memory after the index is built
+  auto _ = llvm::make_scope_exit(trimMemory);
+
----------------
I think this should like in ClangdLSPServer, triggered by all of:
 - various events: outgoing notification, incoming notification, onBackgroundIndexProgress()
 - an option specified by ClangdMain (I think it might even be cleanest to inject a "PeriodicMemoryCleanup" function via ClangdLSPServer::Options)
 - a timer having expired since the last time we called this - maybe a minute?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452