On Thu, 2 Nov 2023 17:42:29 GMT, Thomas Stuefe <stu...@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 another windows error If the command is only usable on Linux, and only registered on Linux, do we really need the empty definitions in `memMapPrinter_<os>.cpp`? At present it seems we only need it because all the implementation code is not isolated to `ifdef LINUX`. src/hotspot/share/runtime/thread.cpp line 474: > 472: osthread()->print_on(st); > 473: } > 474: st->print("stack=[" PTR_FORMAT ", " PTR_FORMAT ")", > p2i(stack_end()), p2i(stack_base())); We already report the thread stack elsewhere, so is this going to duplicate the information? src/hotspot/share/runtime/vmOperation.hpp line 121: > 119: template(JvmtiPostObjectFree) \ > 120: template(RendezvousGCThreads) \ > 121: template(SystemMap) I can't spot the VMop for this. ?? src/hotspot/share/services/diagnosticCommand.cpp line 1159: > 1157: } > 1158: > 1159: SystemMapDCmd::SystemMapDCmd(outputStream* output, bool heap) : Shouldn't this all be inside `ifdef LINUX`? You don't need `os::realpath` if you do that. src/hotspot/share/services/diagnosticCommand.cpp line 1193: > 1191: output()->print_cr("Memory map dumped to \"%s\".", name); > 1192: } else { > 1193: output()->print_cr("Failed to open \"%s\" for writing.", name); Don't you want to report errno here? src/hotspot/share/services/diagnosticCommand.hpp line 988: > 986: static const char* name() { return "System.map"; } > 987: static const char* description() { > 988: return "Prints an annotated process memory map of the VM process."; Should this specify "Linux Only" ? src/hotspot/share/services/diagnosticCommand.hpp line 1007: > 1005: static const char* name() { return "System.dump_map"; } > 1006: static const char* description() { > 1007: return "Dumps an annotated process memory map to an output file."; Again "Linux Only"? ------------- Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1721842005 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1387576827 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1387580917 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1387584901 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1387585667 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1387572049 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1387577576
