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

[Thomas Schatzl]

Hi Sangheon,

On Fri, 2018-11-09 at 13:17 -0800, sangheon.kim at oracle.com wrote:
> Hi Thomas,
> 
> Thanks for looking at this.
> 
> On 11/7/18 3:26 PM, Thomas Schatzl wrote:
> > 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.
> 
> Okay, applied this at the new webrev.
> My initial version was something like your suggestion, but dropped as
> I  understood only 1 place needs it. But it was wrong as you pointed
> below.
> > 
> > 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?
> 
> Yes, same treatment is needed there too.
> Before filing this bug, I thought we don't need this for the
> auxiliary data structures, but I was wrong.
> 
> webrev: http://cr.openjdk.java.net/~sangheki/8211735/webrev.2/
> Testing: hs-tier1~3.
> 
> PS. I'm also fine with going with webrev.1 style with auxiliary 
> structure treated.

After some thinking, and also looking at JDK-8213927 I would prefer the
webrev.1 style change. Just put the code into a static helper function
in g1CollectedHeap.cpp.

The reason is that I believe that the large page code, and determining
for which purpose we can assume large pages and the exceptions when
not, is a bit messed up or at least underdocumented. Maybe partially
because of the Linux large page mess :)

>From what I understand it seems that there is quite a bit of confusion
in the code what "can_commit_large_page_memory()" actually means; I
believe at least some uses are incomplete (i.e. not considering the
"special" flag) or not giving any reason why they do not consider it. I
do not completely understand why UseTransparentHugePages implicitly
enables UseLargePages either.

I think at this point we should not add to the ReservedSpace confusion
any further by adding more ReservedSpace methods. Sorry for initially
suggesting that.

I would also prefer to use name for that function like
"preferred/actual_commit_page_size(ReservedSpace rs)" to make it clear
it is a page size for committing and not for any other purpose (which
is the trap the bug exposed by JDK-8213927 fell into).

Other than that the actual algorithm to determine the memory commit
page size is good.

Thanks,
  Thomas