[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8318636: Add jcmd to print annotated process memory map [v6]
From:       Gerard Ziemski <gziemski () openjdk ! org>
Date:       2023-10-31 17:33:41
Message-ID: nPcikdsLhnQpgM74kuGde221X3tFO0QOKp3cNpRApIc=.840e508c-96bd-4723-b103-e6127fd27c5a () github ! com
[Download RAW message or body]

On Sat, 28 Oct 2023 13:04:05 GMT, Thomas Stuefe <stuefe@openjdk.org> wrote:

> > Analysts and supporters often use /proc/xx/maps to make sense of the memory \
> > footprint of a process. 
> > Interpreting the memory map correctly can help when used as a complement to other \
> > tools (e.g. NMT). There even exist tools out there that attempt to annotate the \
> > process memory map with JVM information. 
> > That, however, can be more accurately done from within the JVM, at least for \
> > mappings originating from hotspot. After all, we can correlate the mapping \
> > information in NMT with the VMA map. 
> > Example output (from a spring petstore run):
> > 
> > [example_system_map.txt](https://github.com/openjdk/jdk/files/13179054/example_system_map.txt)
> >  
> > This patch adds the VM annotations for VMAs that are *mmaped*. I also have an \
> > experimental patch that works with malloc'ed memory, but it is not ready yet for \
> > consumption and I leave that part of for now.
> 
> Thomas Stuefe has updated the pull request incrementally with one additional commit \
> since the last revision: 
> fix various builds

Just a couple of issues/questions.

This is a very nice feature we're adding. Thank you!

src/hotspot/os/linux/memMapPrinter_linux.cpp line 59:

> 57:   void print_OS_specific_details(outputStream* st) const override {
> 58:     st->print("%s %s ", _info.prot, _info.offset);
> 59:   }

If that's all this is doing, do we really need it?

src/hotspot/os/linux/memMapPrinter_linux.cpp line 83:

> 81:     char line[linesize];
> 82:     while(fgets(line, sizeof(line), f) == line) {
> 83:       line[sizeof(line) - 1] = '\0';

What would happen if the read line is empty, i.e. sizeof(line)==0, can that happen?

src/hotspot/share/nmt/memMapPrinter.cpp line 79:

> 77:   const void* const min = MAX2(from1, from2);
> 78:   const void* const max = MIN2(to1, to2);
> 79:   return min < max;

I had to rewrite it as:

`return MAX2(from1, from2) < MIN2(to1, to2);`

to make sure I understand it, or better yet, why not have it as a macros?

`#define RANGE_INTERSECT(min, max) (return MAX2(from1, from2) < MIN2(to1, to2))`

 MAX2 and MIN2 are macros already and we're not doing anything fancy here.

src/hotspot/share/nmt/memMapPrinter.cpp line 89:

> 87:   // NMT deadlocks (ThreadCritical).
> 88:   Range* _ranges;
> 89:   MEMFLAGS* _flags;

Why did you decide to have two separate memory chunks?

I think I'd have used a struct to hold Range* and MEMFLAGS* and use that struct in a \
single array.

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

Changes requested by gziemski (Committer).

PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1704722251
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1377905955
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1376585794
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1377917390
PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1377926176


[prev in list] [next in list] [prev in thread] [next in thread]