RFR (xs): 8211735: Wrong heap mapper can be selected with UseLargePages on G1

[Thomas Schatzl]

Hi,

On Wed, 2018-11-07 at 14:38 -0800, sangheon.kim at oracle.com wrote:
> Hi Kim,
> 
> Thanks for your review.
> 
> On 11/7/18 1:47 PM, Kim Barrett wrote:
> > > On Nov 7, 2018, at 4:22 PM, sangheon.kim at oracle.com wrote:
> > > 
> > > Hi all,
> > > 
> > > Can I have reviews for this tiny patch fixes wrong heap mapper
> > > selection with UseLargePages on G1?

It is unfortunate that we can't ask the ReservedSpace directly whether
it supports large pages.

Also, isn't there the same problem when determining the heap mapper for
the auxiliary data structures in create_aux_memory_mapper, and
shouldn't there be similar changes there?

Thanks,
  Thomas

> > > 
> > > When we enable UseLargePages but failed allocating with large
> > > page, G1 is not correctly reflecting it.
> > > This situation only happens when G1HeapRegionSize is less than or
> > > equal to large page size.
> > > e.g. G1HeapRegionsSize=1MB, default page size=4KB, large page
> > > size=2MB. G1 selects G1RegionsSmallerThanCommitSizeMapper
> > > (G1HeapRegionSize < large page size, at
> > > G1RegionToSpaceMapper::create_mapper()) but actually we are using
> > > default page(failed allocating with large page) so
> > > G1RegionsLargerThanCommitSizeMapper should be selected.
> > > Eventually G1 commits memory with not optimal way(it works) and
> > > pages will not touched properly when AlwaysPreTouch option is
> > > enabled.
> > > 
> > > CR: https://bugs.openjdk.java.net/browse/JDK-8211735
> > > Webrev: http://cr.openjdk.java.net/~sangheki/8211735/webrev.0
> > > Testing: hs-tier1, hs-tier2, hs-tier3
> > 
> > -----------------------------------------------------------------
> > -------------
> > src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> > 1663     if (os::can_commit_large_page_memory() ||
> > 1664         // If we want large page and current os doesn't
> > support committing large page memory,
> > 1665         // we manage differently at ReservedSpace and it is
> > called 'special'.
> > 1666         // If we fail to set 'special', we reserved memory
> > without large page.
> > 1667         (!os::can_commit_large_page_memory() &&
> > heap_rs.special())) {
> > 
> > That's really confusing.
> > Since "x || (!x && y) => x || y", this should be
> > 
> >    if (os::can_commit_large_page_memory() || heap_rs.special()) {
> > 
> > and update the comment accordingly.
> 
> You are right.
> 
> Webrev: http://cr.openjdk.java.net/~sangheki/8211735/webrev.1
> 
> Thanks,
> Sangheon
> 
> 
> > 
> > -----------------------------------------------------------------
> > -------------
> > 
> 
>