RFR: JDK-8301749: Tracking malloc pooled memory size

[Thomas Stuefe]

On Tue, 7 Feb 2023 15:00:14 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

> This PR implements `os::malloc_info` by calling into glibc's `malloc_info` and provides a JCmd for asking the JVM to call `malloc_info` and print out the results. The user can make an informed decision regarding heap trimming by parsing the output of the JCmd. I wanted to avoid parsing the XML inside of the JVM , as this functionality may turn out to be "good enough". A future enhancement would be for the JVM to make this informed decision by parsing the output. Another may be to present this with other data, for example in NMT.
> 
> @tstuefe, I think you'd be interested in this PR as it builds upon your `trim_native_heap` functionality.
> 
> I'm using weak symbols (as per ELF) in order to compile this for both GLibc and Musl.
> 
> The original bug description is this:
> 
>>Memory not allocated via NMT and uncommitted memory contributes to a discrepancy between NMT and RSS.
> It can be hard to distinguish between these cases.
> If the VM can determine the amount of pooled memory by malloc it would help with both determining if a trim_native_memory is needed, or there are some allocations happening outside of NMT.
>>
>>It would be good if the VM can summarize the malloc pooled memory by using e.g. malloc_info(3).
>>
>>We can thus more precise account for RSS value.

@jdksjolen,

interesting. I considered malloc_info() too, but did not feel like parsing XML.

Generally, I would prefer if you keep it closer to what os::get_mallinf() is doing:

- only define and call that API for ifdef GLIBC
- resolve the traditional way via function pointers (see our mallinf()/mallinf2() wrappers)
- have the dcmd execute functions guarded with GLIBC, and an else branch saying its not glibc

This also needs tests. See existing test for trim_native_heap. I would also like a test for muslc to test the stub functionality (its missing from TrimLibcHeapTest.java, which I will add).

src/hotspot/os/linux/mallocInfoDcmd.cpp line 26:

> 24: 
> 25: #include "precompiled.hpp"
> 26: #include "runtime/os.inline.hpp"

why this? why not os_linux.hpp?

src/hotspot/os/linux/mallocInfoDcmd.cpp line 28:

> 26: #include "runtime/os.inline.hpp"
> 27: #include "mallocInfoDcmd.hpp"
> 28: #include "utilities/debug.hpp"

What do you use from debug.hpp?

src/hotspot/os/linux/mallocInfoDcmd.cpp line 38:

> 36:   char* buf;
> 37:   size_t size;
> 38:   ALLOW_C_FUNCTION(::open_memstream, FILE* stream = ::open_memstream(&buf, &size);)

I wondered how open_memstream communicates buffer growth (which may come with re-location). Then I realized that it just modifies the buffer start address the caller gives it in-place. What a weird API :)

You should handle error returns from open_memstream.

src/hotspot/os/linux/mallocInfoDcmd.cpp line 39:

> 37:   size_t size;
> 38:   ALLOW_C_FUNCTION(::open_memstream, FILE* stream = ::open_memstream(&buf, &size);)
> 39:   if (!os::Linux::malloc_info(stream)) {

Style nit, can you change this to `if (os::Linux::malloc_info(stream) == 0)` ? Using `!` it reads like the failure branch.

src/hotspot/os/linux/mallocInfoDcmd.hpp line 36:

> 34:   MallocInfoDcmd(outputStream* output, bool heap) : DCmd(output, heap) {}
> 35:   static const char* name() {
> 36:     return "System.malloc_info";

Nit, since the other one is called trim_native_heap, why not "native_heap_info"?

src/hotspot/os/linux/os_linux.cpp line 118:

> 116: 
> 117: // Forward (re-)declare malloc_info in order to support Alpine Linux.
> 118: int __attribute__((weak)) malloc_info(int options, FILE *stream);

Not needed if you do it like os::get_mallinfo() and dlsym the API as proposed.

src/hotspot/os/linux/os_linux.cpp line 227:

> 225:     return ::malloc_info(0, stream);
> 226:   }
> 227:   return 0;

return -1.

You aim to mimic the malloc_info() API at os:: level, so you should return -1 in case malloc_info failed, including failure do to resolving issues.

src/hotspot/os/linux/os_linux.hpp line 199:

> 197:   // otherwise does nothing and returns 0.
> 198:   static int malloc_info(FILE* stream);
> 199: 

Could you put this at the end of the file, alongside os::get_mallinfo(), and within its ifdef GLIBC boundaries?

-------------

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12455