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

[sangheon.kim at oracle.com]

Thanks Kim for the review.

Sangheon


On 11/28/18 1:55 PM, Kim Barrett wrote:
>> 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.
>