[PATCH] D93452: [clangd] Trim memory periodically

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Awesome! Some nits on naming and stuff, but the only real substantial question for me is the exact availability/behavior of the command-line flag. Even there, as long as the default behavior is to trim when build with glibc, the details are less important.
Let me know when you want me to land it!

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

> In the meantime, I've done some testing with tcmalloc and jemalloc.
> `jemalloc` does very good in this situation, the RSS goes down as soon as the memory is not required anymore:
> `tcmalloc` does fine, but is not ideal, the RSS does not grow to an unreasonable size, but does not go down when we release resources.

Thanks for this! I agree this is basically tcmalloc working as designed, it tends to sit at the high-water mark.

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

> Let me know if I should do anything concerning these files.
>
> - clang-tools-extra/clangd/test/lit.site.cfg.py.in

This is only needed if we want to make lit tests run conditionally on whether the CMake variable is enabled. (For various reasons, I think testing this at all is probably more trouble than it's worth).

> - llvm/utils/gn/secondary/clang-tools-extra/clangd/BUILD.gn
> - llvm/utils/gn/secondary/clang-tools-extra/clangd/test/BUILD.gn

These are for the alternate `gn` build system, which is maintained by its users (commits are not expected to keep it working). It would be reasonable/friendly to blindly add `"CLANGD_ENABLE_MEMORY_CLEANUP=1"` to both files so that gn always builds in that configuration. FWIW I almost always forget/don't bother, and that's totally acceptable too.



================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:17
 
+option(CLANGD_ENABLE_MEMORY_CLEANUP "Enable active memory cleanup for Clangd." ON)
+
----------------
In keeping with reducing the abstraction level, maybe we could call this CLANGD_MALLOC_TRIM and note "(only takes affect when using glibc)"?


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:504
 
+#if CLANGD_ENABLE_MEMORY_CLEANUP
+opt<bool> EnableMemoryCleanup{
----------------
currently we have the flag or not depending on the cmake flag, and it does something or not depending on whether we're using glibc.

I'd suggest rather we should have the flag if we have glibc (i.e. if it does something), and its *default* should depend on the cmake flag.


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:504
 
+#if CLANGD_ENABLE_MEMORY_CLEANUP
+opt<bool> EnableMemoryCleanup{
----------------
sammccall wrote:
> currently we have the flag or not depending on the cmake flag, and it does something or not depending on whether we're using glibc.
> 
> I'd suggest rather we should have the flag if we have glibc (i.e. if it does something), and its *default* should depend on the cmake flag.
It would also be nice to reduce the number of separate #ifdef sections (currently 4).

I'd suggest something like:

```
#ifdef __GLIBC__
opt<bool> EnableMallocTrim{
...
   init(CLANGD_MALLOC_TRIM),
};
static std::function<void()> MallocTrimFunc() {
  if (!EnableMallocTrim)
    return nullptr;
  return []{ ... };
}
#else
static std::function<void()> MallocTrimFunc() {
  return nullptr;
}
#endif
```

then later we can assign to opts without an ifdef or other condition


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:600
+  constexpr size_t MallocTrimPad = 20'000'000;
+  return []() { malloc_trim(MallocTrimPad); };
+#else
----------------
now we don't have the abstraction, we can have our logging back:

```
if (malloc_trim(MallocTrimPad))
  vlog("Released memory via malloc_trim");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452