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

[sangheon.kim at oracle.com]

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


>
>
> Thanks,
> Sangheon
>
>
>>
>> Thanks,
>>    Thomas
>>
>>
>