On Thu, 15 Aug 2024 13:45:16 GMT, Simon Tooke <sto...@openjdk.org> wrote:
> This is a port of [JDK-8318636](https://github.com/openjdk/jdk/pull/16301) to > Windows. > > System.map and System.dump_map are implemented using the Windows API and > provide roughly the same information in the same format. Most of the heavy > lifting was implemented by @tstuefe in #16301 - this PR adds the Windows > implementation and enables the common code for Windows 64 bit. > > [Sample output (with NMT > enabled)](https://github.com/user-attachments/files/16663332/vm_memory_map_760.txt) Hi Simon, this is nice, thank you! Mostly nits. src/hotspot/os/windows/memMapPrinter_windows.cpp line 3: > 1: /* > 2: * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights > reserved. > 3: * Copyright (c) 2023, 2024, Red Hat, Inc. and/or its affiliates. New files are added with the current copyright year, unless they are the result of moving. src/hotspot/os/windows/memMapPrinter_windows.cpp line 32: > 30: > 31: #include <limits> > 32: #include <iostream> Please use C-headers. We only use C++ headers sparingly. Why do you include these headers? src/hotspot/os/windows/memMapPrinter_windows.cpp line 63: > 61: } > 62: > 63: const char* getProtectString(char* buffer, size_t bufsiz, DWORD prot) > { Style: In hotspot we use camel case for classes, lower_case_underscore for method, functions etc. src/hotspot/os/windows/memMapPrinter_windows.cpp line 69: > 67: const int IP = 3; > 68: int idx = 4; > 69: strncpy_s(buffer, bufsiz, "---- ", 7); We tend to use os::snprintf or jio_snprintf for this. I find strncpy_s a bit fragile since one needs to update both string literal and char count when changing things. src/hotspot/os/windows/memMapPrinter_windows.cpp line 98: > 96: buffer[idx++] = 'n'; > 97: } else if (prot != 0) { > 98: snprintf(buffer, bufsiz, "(0x%x)", prot); This could truncate for the longest possible value (16+4 chars). src/hotspot/os/windows/memMapPrinter_windows.cpp line 111: > 109: buffer[idx++] = 'W'; > 110: } > 111: buffer[idx] = 0; Please give us an assert here for idx < bufsiz src/hotspot/os/windows/memMapPrinter_windows.cpp line 114: > 112: return buffer; > 113: } > 114: const char* getStateString(char* buffer, size_t bufsiz, > MEMORY_BASIC_INFORMATION& mInfo) { We have a utility class for printing to a limited buffer (stringStream). It takes care of buffer limits and zero termination for you. I would use that one where possible. stringStream ss(buffer, bufsiz); if (mInfo.State == MEM_COMMIT) { ss.put('c'); } ... else { ss.print("0x%x", mInfo.State); } return buffer; src/hotspot/os/windows/memMapPrinter_windows.cpp line 145: > 143: size_t _total_region_size; // combined resident set size > 144: size_t _total_committed; // combined committed size > 145: size_t _total_reserved; // combined shared size I would like us to be careful with terminology since that can get confusing quickly when jumping between OS code and generic code. In hotspot terminology, "reserved" are parts of the address space that have been allocated for the process, *regardless of commit state*. So, the whole region would count as "reserved", with some pages being also committed, and some not. But on Windows, the MEM_RESERVE state is only given to uncommitted regions, so there is a subtle difference. I would circumvent the problem by counting only: - total size (as in sum of region sizes that are not MEM_FREE) - committed size (as in sum of region sizes that are MEM_COMMIT) src/hotspot/os/windows/memMapPrinter_windows.cpp line 162: > 160: outputStream* st = session.out(); > 161: os::print_os_info(st); > 162: os::print_memory_info(st); The "danger" here is that both of these functions may bloat in the future and print a lot more than today; and that we may want to use this printout in places where we also print out OS info and memory info, and therefore get duplicated printout (e.g. hs-err printing or jcmd VM.info). But I would print out the number of mappings you counted, and the total size of committed regions. src/hotspot/os/windows/memMapPrinter_windows.cpp line 243: > 241: printer.print_header(); > 242: > 243: MEMORY_BASIC_INFORMATION mInfo; Just to be sure, I would zero out mInfo. src/hotspot/os/windows/memMapPrinter_windows.cpp line 246: > 244: MappingInfo info; > 245: > 246: for (char* ptr = 0; VirtualQueryEx(hProcess, ptr, &mInfo, > sizeof(mInfo)) == sizeof(mInfo); ptr += mInfo.RegionSize) { I would add a failsafe breakout in case we made a thinking error somewhere or the address space is insanely fragmented. Maybe break out after a million regions? Or, after a certain time (e.g. 10 seconds). I also would assert for RegionSize > 0 src/hotspot/share/services/diagnosticCommand.cpp line 1207: > 1205: const char* absname = os::Posix::realpath(name, tmp, sizeof(tmp)); > 1206: name = absname != nullptr ? absname : name; > 1207: #endif I wince a bit a the new ifdefs. Possibly for a separate RFE, it would be nice to have a function os::realpath() that uses realpath(2) on POSIX platforms, _fullpath on Windows. ------------- PR Review: https://git.openjdk.org/jdk/pull/20597#pullrequestreview-2247042750 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722735615 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722743452 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722745964 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722766129 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722763766 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722757477 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722770690 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722783949 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722795939 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722809524 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722807479 PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1722801573
