RFR: 8313306: More sensible reservation logging [v8]

[Thomas Stuefe]

On Fri, 2 Feb 2024 09:34:16 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> This PR implements more specialized logs for virtual memory APIs in the “os” namespace. It uses “os” and “map” as log tags using unified JVM logging as introduced in JEP 158 (https://openjdk.org/jeps/158). 
>> 
>> As far as testing is concerned, I have added a regression test to verify the logging is working accordingly.
>
> Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision:
> 
>   adding test flag

Good, it is getting somewhere. Small nits left.

src/hotspot/os/aix/os_aix.cpp line 299:

> 297:     if (::disclaim(p, maxDisclaimSize, DISCLAIM_ZEROMEM) != 0) {
> 298:       ErrnoPreserver ep;
> 299:       log_trace(os,map)("disclaim failed: " RANGEFMT " errno=(%s)", RANGEFMTARGS(p, maxDisclaimSize), os::strerror(ep.saved_errno()));

Lets reduce things a bit.

Please give ErrnoPreserver a utility function like this:

os.hpp

const char* err_text() const;


that returns os::strerror, then use that one instead in all call sites.

src/hotspot/os/aix/os_aix.cpp line 1476:

> 1474:       log_trace(os,map)(RANGEFMT " is not a sub "
> 1475:               "range of " RANGEFMT, RANGEFMTARGS(p, s),
> 1476:               RANGEFMTARGS(addr, size));

Since you are here, could you change this to not trace at all, but move the message into the assert? Also, if we use `assert` or `guarantee` with just "false", we may just as well do `fatal`. (This code is from me, I think from around 2005-6, asserts were different back then).

So, something like this:


fatal(RANGEFMT " is not a sub "
              "range of " RANGEFMT, RANGEFMTARGS(p, s),
              RANGEFMTARGS(addr, size));

src/hotspot/os/aix/os_aix.cpp line 1608:

> 1606:   if (addr == (char*)-1) {
> 1607:     ErrorPreserver ep;
> 1608:     log_trace(os,map)("Failed to attach segment at " PTR_FORMAT " (errno=%s).", p2i(requested_addr), os::strerror(ep.saved_errno()));

line break please (here, and in similarly long lines)

src/hotspot/os/linux/os_linux.cpp line 3640:

> 3638:       char* const end = start + extra_size;
> 3639:       if (start_aligned > start) {
> 3640:         ::munmap(start, start_aligned - start);

Please do something like this:

if (start_aligned > start) {
  const size_t l = start_aligned - start;

then use l instead in the following lines, and remove the size_t cast.

Same below for end_aligned.

And please break the lines somewhere.

src/hotspot/share/runtime/os.cpp line 1826:

> 1824:   if (result != nullptr) {
> 1825:     MemTracker::record_virtual_memory_reserve((address)result, bytes, CALLER_PC);
> 1826:     log_debug(os,map)("Reserved " RANGEFMT, RANGEFMTARGS(result, bytes));

nit, space after comma (here, and other places)

src/hotspot/share/runtime/os.hpp line 157:

> 155: 
> 156: // Preserve errno across a range of calls
> 157: 

Indentation: 2 spaces, not four, no tabs. In general, look at the surrounding code - when in Rome...

 If possible, adjust your IDE accordingly, at least for hotspot sources.

src/hotspot/share/utilities/globalDefinitions.hpp line 382:

> 380: #define PROPERFMTARGS(s)      byte_size_in_proper_unit(s), proper_unit_for_byte_size(s)
> 381: 
> 382: #define RANGEFMT                      "[" PTR_FORMAT " - " PTR_FORMAT "), (" SIZE_FORMAT " bytes)"

Please adjust indentations to line up

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

Changes requested by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17356#pullrequestreview-1864385327
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1479294700
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1479288915
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1479298707
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1479292391
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1479300583
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1479295798
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1479296793