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

[Thomas Schatzl]

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