RFR: JDK-8076542: G1 does not print heap page size information with -XX:+TracePageSizes

[David Lindholm]

Hi,

Thanks Thomas! A new webrev based on your feedback is available here:

http://cr.openjdk.java.net/~david/JDK-8076542/webrev.01/


We could have a discussion about what to print when TracePageSizes is 
enabled (for all collectors), but I think it could be done outside this bug.


Regards,
David

On 2015-05-07 15:53, Thomas Schatzl wrote:
> Hi,
>
> On Thu, 2015-05-07 at 15:15 +0200, Bengt Rutisson wrote:
>> Hi David,
>>
>> On 06/05/15 14:11, David Lindholm wrote:
>>> Hi,
>>>
>>> Please review this patch that adds a call to os::trace_page_sizes()
>>> for G1, which prints heap page size info if run with -XX:+TracePageSizes
>>>
>>>
>>> Webrev: http://cr.openjdk.java.net/~david/JDK-8076542/webrev.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8076542
>> 1864   ReservedSpace heap_rs = Universe::reserve_heap(max_byte_size,
>> 1865 heap_alignment);
>> 1866
>> 1867   size_t page_sz = os::page_size_for_region_aligned(max_byte_size, 1);
>> 1868   os::trace_page_sizes("g1 heap",
>> collector_policy()->min_heap_byte_size(),
>> 1869                        max_byte_size, page_sz,
>> 1870                        heap_rs.base(),
>> 1871                        heap_rs.size());
>>
>> I think it looks odd that we look up the page size that we print instead
>> of actually getting the page size that was used.
>>
> Thinking about "TracePageSizes" and what is printed, it seems that it
> confuses page size used for commit and reservation alignment.
>
>> What I mean is that if we change the implementation of
>> Universe::reserve_heap() to do something differently than using
>> os::page_size_for_region_aligned() then this logging will suddenly be
>> broken. I would prefer if we could somehow communicate what page size
>> that was actually used. Or is it even the case that it is
>> Universe::reserve_heap() that should do the call to os::trace_page_sizes()?
> As mentioned above, it is strange to me to talk about page size when
> reserving a memory area. I am not sure if we can or should fix the
> output of TracePageSizes.
>
> Actual page size used for committing is determined in line 1883 btw:
>
>    G1RegionToSpaceMapper* heap_storage =
>      G1RegionToSpaceMapper::create_mapper(g1_rs,
>                                           g1_rs.size(),
>                                           UseLargePages ?
> os::large_page_size() : os::vm_page_size(),            // <!---
>                                           HeapRegion::GrainBytes,
>                                           1,
>                                           mtJavaHeap);
>
> Which means that the value actually used is different to the value
> calculated in this change; it should be the same in all cases though,
> but it would be nice to factor out this calculation and use it there and
> when calling os::trace_page_sizes.
>
> Also the kind string currently does not match the others printed
> (created in create_aux_mapper()).
>
>> Another thing, which is not really related to your change. There are two
>> version of os::trace_page_sizes(). I couldn't find any user of this version:
>>
>> os::trace_page_sizes(const char* str, const size_t* page_sizes, int count)
>>
>> Can that one be removed?
> It is used to print out all available page sizes on Solaris.
> (os_solaris.cpp:3058)
>
> Thanks,
>    Thomas
>
>