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

[Kim Barrett]

> On Nov 20, 2018, at 11:54 PM, sangheon.kim at oracle.com wrote:
> 
> Hi all,
> 
> On 11/19/18 11:48 AM, sangheon.kim at oracle.com wrote:
>> 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
> Sorry, I have to post a new version for addressing aux. data structure case as well.
> 
> webrev:
> http://cr.openjdk.java.net/~sangheki/8211735/webrev.4 (full)
> http://cr.openjdk.java.net/~sangheki/8211735/webrev.4_to_3 (incremental)
> Testing: hs-tier1 ~ 3
> 
> Thanks,
> Sangheon

Looks good.