On Tue, 20 Aug 2024 06:06:39 GMT, Thomas Stuefe <stu...@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
