RFR: 8319873: Add windows implementation for jcmd System.map and System.dump_map [v2]

[Simon Tooke]

On Tue, 20 Aug 2024 06:06:39 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Simon Tooke has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   changes from tstuefe review
>
> 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.

Done.

> 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?

I honestly have no clue how iostreams crept into there.  Gone!  and C header now used.

> 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.

dont - and I noticed tabs were inconsistent too.

> 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

I've rewritten this code to use stringStream instead.

> 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)

I've done this and revised the legend, too.

> 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.

done.

> 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

done and done.

> 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.

I agree, and was surprised this wasn't originally implemented.  I'll prepare a second PR.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1724976340
PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1724976052
PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1724975064
PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1724974250
PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1724972279
PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1724970587
PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1724970953
PR Review Comment: https://git.openjdk.org/jdk/pull/20597#discussion_r1724971567