RFR: 8313306: More sensible reservation logging

[David Holmes]

On Wed, 10 Jan 2024 21:33:26 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.

I've taken a first pass through this and have some issues with the actual code.

Thanks.

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

> 1813:   if (result != nullptr) {
> 1814:     MemTracker::record_virtual_memory_reserve(result, bytes, CALLER_PC, flags);
> 1815:     log_debug(os,map)("Reserved [" INTPTR_FORMAT " - " INTPTR_FORMAT"] (%zu bytes).", p2i(result), p2i(result + bytes), bytes);

Space needed after INTPTR_FORMAT in `INTPTR_FORMAT"` - this applies to multiple locations

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

> 1815:     log_debug(os,map)("Reserved [" INTPTR_FORMAT " - " INTPTR_FORMAT"] (%zu bytes).", p2i(result), p2i(result + bytes), bytes);
> 1816:   } else {
> 1817:     log_info(os,map)("Reserve failed (%zu bytes), errno %d %s", bytes, get_last_error(), strerror(get_last_error()));

This looks suspicious. First, it isn't clear that `get_last_error()` will actually return the error from `pd_reserve_memory` as you can't tell what might have intervened. Second, the second call may well return a different value to the first, again depending on how this all gets expanded and evaluated. Third `strerror` is only for converting `errno` to a string and on Windows `get_last_error()` may not be returning `errno` but a Windows specific error code.

test/hotspot/jtreg/runtime/os/TestMemoryAllocationLogging.java line 2:

> 1: /*
> 2:  * Copyright (c) 2023 Oracle and/or its affiliates. All rights reserved.

An Oracle copyright is not required in this new file as Oracle did not create it. But for reference note that Oracle copyrights require a comma after the year.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17356#pullrequestreview-1817468769
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1449892081
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1449901626
PR Review Comment: https://git.openjdk.org/jdk/pull/17356#discussion_r1449905233