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

[sangheon.kim at oracle.com]

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.

Thanks,
Sangheon

>
> Thanks,
>    Thomas
>
>>>> When we enable UseLargePages but failed allocating with large
>>>> page, G1 is not correctly reflecting it.
>>>> This situation only happens when G1HeapRegionSize is less than or
>>>> equal to large page size.
>>>> e.g. G1HeapRegionsSize=1MB, default page size=4KB, large page
>>>> size=2MB. G1 selects G1RegionsSmallerThanCommitSizeMapper
>>>> (G1HeapRegionSize < large page size, at
>>>> G1RegionToSpaceMapper::create_mapper()) but actually we are using
>>>> default page(failed allocating with large page) so
>>>> G1RegionsLargerThanCommitSizeMapper should be selected.
>>>> Eventually G1 commits memory with not optimal way(it works) and
>>>> pages will not touched properly when AlwaysPreTouch option is
>>>> enabled.
>>>>
>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8211735
>>>> Webrev: http://cr.openjdk.java.net/~sangheki/8211735/webrev.0
>>>> Testing: hs-tier1, hs-tier2, hs-tier3
>>> -----------------------------------------------------------------
>>> -------------
>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>>> 1663     if (os::can_commit_large_page_memory() ||
>>> 1664         // If we want large page and current os doesn't
>>> support committing large page memory,
>>> 1665         // we manage differently at ReservedSpace and it is
>>> called 'special'.
>>> 1666         // If we fail to set 'special', we reserved memory
>>> without large page.
>>> 1667         (!os::can_commit_large_page_memory() &&
>>> heap_rs.special())) {
>>>
>>> That's really confusing.
>>> Since "x || (!x && y) => x || y", this should be
>>>
>>>     if (os::can_commit_large_page_memory() || heap_rs.special()) {
>>>
>>> and update the comment accordingly.
>> You are right.
>>
>> Webrev: http://cr.openjdk.java.net/~sangheki/8211735/webrev.1
>>
>> Thanks,
>> Sangheon
>>
>>
>>> -----------------------------------------------------------------
>>> -------------
>>>
>>
>