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

[sangheon.kim at oracle.com]

Hi Thomas,

On 11/15/18 4:07 AM, Thomas Schatzl wrote:
> 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.
Okay.

>
> 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 :)
Right!!!

>
>  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 agree especially not adding more confusion at ReservedSpace. :)

>
> 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).
I would prefer to use 'actual_reserved_page_size()' as it is reserved 
and not committed yet.
Or as you say it is for committing so if the name has such meaning, I am 
okay too. 'xxx_commit_page_size()' feels like 'committed' to me.

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

(We had a chat about this but adding here for the record)
Same treatment at the auxiliary data structure is not needed.
In the previous email I said we need the same one for the aux. 
structures because I understood the code:
if the aux data structure size is smaller than current large page size, 
we should align up and then use large page.
But current code is intentionally not use larger page, if the aux data 
structure size is less.
e.g. aux. data structure size=3mb, large page size=4mb, currently we use 
default page in this case.

So this new webrev 3 has a new static helper function added but only 
used for heap.

webrev:
http://cr.openjdk.java.net/~sangheki/8211735/webrev.3 (full)
http://cr.openjdk.java.net/~sangheki/8211735/webrev.3_to_2 (incremental)
Testing: hs-tier1 ~ 3


Thanks,
Sangheon


>
> Thanks,
>    Thomas
>
>