RFR: JDK-8310107: os::trace_page_sizes_for_requested_size should name alignment as requested page size [v3]

[David Holmes]

On Fri, 16 Jun 2023 18:56:23 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Somewhat trivial renaming patch.
>> 
>> `os::trace_page_sizes_for_requested_size()` names "alignment" what is actually the requested page size. Historically, the distinction between "alignment" and "page size" in memory reservation code has not been clear. We often use "alignment" where we mean "page size". OTOH, ReservedSpace constructor takes a "preferred page size" argument, which it then uses as both alignment request and pagesize request, meaning that even if it fails to get the desired page size it attempts to satisfy the alignment.
>> 
>> Clearing this up would be a larger effort. Here, I propose to rename "alignment" to "requested page size" in `os::trace_page_sizes_for_requested_size()` since it clearer conveys the difference between requested and actual page size, which is the important bit for pagesize logging.
>> 
>> ---
>> 
>> Patch renames "alignment" to "requested pagesize" in printout and code.
>> 
>> It also reshuffles the parameters somewhat to have requested properties at the beginning, followed by the actual properties of the region.
>> 
>> Printout before, for a failed 2M paged allocation. Notice the difference between requested and actual page size, the latter being labeled as "alignment"):
>> 
>> 
>> OpenJDK 64-Bit Server VM warning: Failed to reserve and commit memory using large pages. req_addr: 0x0000000000000000 bytes: 2097152
>> [0.012s][info][pagesize] Block Offset Table: req_size=2M base=0x00007f3518200000 page_size=4K alignment=2M size=2M
>> 
>> 
>> Printout now:
>> 
>> 
>> OpenJDK 64-Bit Server VM warning: Failed to reserve and commit memory using large pages. req_addr: 0x0000000000000000 bytes: 2097152
>> [0.011s][info][pagesize] Block Offset Table: req_size=2M req_page_size=2M base=0x00007fda0e400000 size=2M page_size=4K
>
> Thomas Stuefe has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Fix test error
>  - remove leftover prototype

Functional changes seem fine. I don't know the tests so can't comment on those other than the seemingly unrelated change to print the summary.

Thanks.

test/hotspot/jtreg/gc/g1/TestLargePageUseForAuxMemory.java line 129:

> 127: 
> 128:         OutputAnalyzer output = new OutputAnalyzer(pb.start());
> 129:         output.reportDiagnosticSummary();

This should come last as if any `output.shouldxxx()` fails then the summary will be printed anyway, so we don't need to see it twice.

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14484#pullrequestreview-1485278057
PR Review Comment: https://git.openjdk.org/jdk/pull/14484#discussion_r1233440581