On Thu, 26 Oct 2023 15:02:53 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 with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - Merge master and solve merge conflicts > - small fixes > - start from VM op; show more thread details > - start Hi, Thank you for this PR. These comments are just a first pass, I haven't finished going through the code. src/hotspot/os/linux/memMapPrinter_linux.cpp line 32: > 30: #include "utilities/globalDefinitions.hpp" > 31: > 32: struct proc_maps_info_t { `struct ProcMapsInfo` please. The `_t` is apparently reserved for POSIX, and I prefer our structs and classes to not look like they're coming from a C library anyway. src/hotspot/os/linux/memMapPrinter_linux.cpp line 34: > 32: struct proc_maps_info_t { > 33: unsigned long long from = 0; > 34: unsigned long long to = 0; Can we not use `uintptr_t` here? scanf directive: https://stackoverflow.com/questions/12228227/how-to-get-pointer-sized-integer-using-scanf src/hotspot/os/linux/memMapPrinter_linux.cpp line 39: > 37: char dev[20 + 1]; > 38: char inode[20 + 1]; > 39: char filename[1024 + 1]; Maybe use `PATH_MAX` here? Potentially +1 to account for null byte. src/hotspot/os/linux/memMapPrinter_linux.cpp line 47: > 45: if (items_read < 2) { > 46: return false; > 47: } Not necessary thanks to the other return value! src/hotspot/os/linux/memMapPrinter_linux.cpp line 72: > 70: "from to " > 71: #else > 72: "from to " 16+1 and 8+1 spaces, depending on how long the addresses are I assume? Your choice, but might be good to note that. src/hotspot/os/linux/memMapPrinter_linux.cpp line 81: > 79: FILE* f = os::fopen("/proc/self/maps", "r"); > 80: if (f != nullptr) { > 81: char line[1024]; This looks like a bug: A line may at most be 1024 characters, but `scan_proc_maps_line` may clearly read more than 1024 characters. Yes, it won't read out of bounds, but it limits the parser unnecessarily. src/hotspot/share/services/memMapPrinter.hpp line 43: > 41: const void* to() const { return _to; } > 42: virtual void print_details_1(outputStream* st) const {} // To be > printed before VM annotations > 43: virtual void print_details_2(outputStream* st) const {} // To be > printed before VM annotations May we please have better names :)? test/hotspot/jtreg/serviceability/dcmd/vm/SystemMapTest.java line 48: > 46: if (output.getOutput().contains("please enable Native Memory > Tracking")) { > 47: have_nmt = false; > 48: } `boolean have_nmt = output.getOutput().contains....` ------------- PR Review: https://git.openjdk.org/jdk/pull/16301#pullrequestreview-1701276028 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374325262 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374327488 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374255012 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374253123 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374318000 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374322993 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374313507 PR Review Comment: https://git.openjdk.org/jdk/pull/16301#discussion_r1374311277
