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

Thu Dec 17 08:40:33 PST 2020

qchateau planned changes to this revision.
qchateau added a comment.

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

It does indeed look like facing the problem from the wrong side (fight the end result instead of finding the root cause) but at that point it does solve an annoying problem. As I investigated this, I found that replacing `DenseMap` and `StringMap` wtih `std::map` mitigates the issue. I think allocating huge chunks of contiguous memory temporarily make this issue worse. Not very useful, but hey, I'm just sharing what I observed. As I previously said, resolving this problem by changing containers or allocators is a lot of work and is not guaranteed to actually solve the issue.

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

It can indeed be called somewhere else, I tried a few different placed before submitting this diff. That can be change easily, it's not a problem. I'd also like to see it at higher level: my first implementation called it periodically, inspired from `maybeExportMemoryProfile`, but I got worried by the background index filling the RAM while the user is not in front of his computer, `onBackgroundIndexProgress` should address this issue.

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

Good point

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452