RFR: 8243315: ParallelScavengeHeap::initialize() passes GenAlignment as page size to os::trace_page_sizes instead of actual page size [v3]

[Stefan Johansson]

On Thu, 19 Nov 2020 09:44:25 GMT, Joakim Nordström <github.com+779991+jaokim at openjdk.org> wrote:

>> ### Description
>> The logging for reserved heap space, printed the `GenAlignment` value instead of page size.
>> Before:
>> <pre>
>> [0.369s][info][gc      ] Using Parallel
>> [0.374s][info][pagesize] Heap:  min=2M max=2M base=0x00000000ffe00000 <b>page_size=512K</b> size=2M
>> </pre>
>> Besides changing to display page size using `os::vm_page_size()`, this fix also adds logging of `SpaceAlignment `,`GenAlignment`, and `HeapAlignment`. 
>> After:
>> <pre>
>> [0.367s][info][pagesize] <b>Alignments:  space_align=512K gen_align=512K heap_align=2M</b>
>> [0.369s][info][gc      ] Using Parallel
>> [0.374s][info][pagesize] Heap:  min=2M max=2M base=0x00000000ffe00000 <b>page_size=4K</b> size=2M
>> </pre>
>> Please advice whether the added logging of alignments is sufficient or irrelevant. There were no signs of either alignment being logged anywhere.
>> ### Testing
>> - Tested manually, and logs are showing with arguments <code>-Xlog:pagesize=info -XX:+UseParallelGC</code>
>> - Tested Tier1, Tier2, Tier3.
>
> Joakim Nordström has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Fixed whitespaces.
>  - Added ParallelInitLogger with alignment logging

Thanks for the update Joakim, some more comments :)

src/hotspot/share/gc/parallel/parallelInitLogger.cpp line 30:

> 28: #include "gc/shared/gcLogPrecious.hpp"
> 29: 
> 30: void ParallelInitLogger::print_gc_specific() {

I see this as heap information so I would suggest using `print_heap` instead and as now print it before the general heap info.

src/hotspot/share/gc/parallel/parallelInitLogger.cpp line 38:

> 36:                      byte_size_in_exact_unit(GenAlignment), exact_unit_for_byte_size(GenAlignment),
> 37:                      byte_size_in_exact_unit(HeapAlignment), exact_unit_for_byte_size(HeapAlignment)
> 38:                      );

The indentation here got a bit of now. I also took a look at the current output and we do not use '=' anywhere else so I suggest going with something like this:
Suggestion:

  log_info_p(gc, init)("Alignments:"
                       " Space " SIZE_FORMAT "%s,"
                       " Generation " SIZE_FORMAT "%s,"
                       " Heap " SIZE_FORMAT "%s",
                       byte_size_in_exact_unit(SpaceAlignment), exact_unit_for_byte_size(SpaceAlignment),
                       byte_size_in_exact_unit(GenAlignment), exact_unit_for_byte_size(GenAlignment),
                       byte_size_in_exact_unit(HeapAlignment), exact_unit_for_byte_size(HeapAlignment));
This would generate output like this:
[0.811s][info][gc,init] Compressed Oops: Enabled (Zero based)                                                                                                                                                                                                                                                        
[0.811s][info][gc,init] Alignments: Space 512K, Generation 512K, Heap 2M                                                                                                                                                                                                                                             
[0.811s][info][gc,init] Heap Min Capacity: 8M                                                                                                                                                                                                                                                                        
[0.811s][info][gc,init] Heap Initial Capacity: 1002M                                                                                                                                                                                                                                                                 
[0.811s][info][gc,init] Heap Max Capacity: 16012M                                                                                                                                                                                                                                                                    
[0.811s][info][gc,init] Pre-touch: Disabled

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 697:

> 695:                         page_size,
> 696:                         rs.base(),
> 697:                         rs.size());

Indentation is one off.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 691:

> 689:         page_size = MIN2(rs.alignment(), os::large_page_size());
> 690:       }
> 691:     }

Since this is exactly the same code as in `actual_reserved_page_size()` in g1CollectedHeap.cpp I think we should lift it out to a helper that can be used by both G1 and Parallel. Not entirely sure where the best location for such helper is, but a static function `ReservedSpace::actual_page_size(ReservedSpace)` could work.

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

Changes requested by sjohanss (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1161