RFR: 8222252 - Java ergonomics limits heap to 128GB with disabled compressed oops

[David Holmes]

Hi Bob,

On 16/05/2019 12:52 am, Bob Vandette wrote:
> I’ve updated the webrev based on your comments.
> 
> http://cr.openjdk.java.net/~bobv/8222252/webrev.02/
> 
> Changes:
> 
> 1. MaxRAM is updated if fractional flags cause us to use OS memory size 
> for heap selection.
> 2. If MaxRAM is specified with the other fractional based flags, we will 
> use MaxRAM as
>     upper limit instead of OS memory size.

That seems fine, but here:

1799         if (FLAG_IS_ERGO(UseCompressedOops) && use_os_mem_limit) {

Shouldn't this also be checking for FLAG_IS_DEFAULT(MaxRAM) ? Or better 
that check would fold into the use_os_mem_limit determination:

   bool use_os_mem_limit = (!FLAG_IS_DEFAULT(MaxRAMPercentage) ||
                            !FLAG_IS_DEFAULT(MaxRAMFraction) ||
                            !FLAG_IS_DEFAULT(MinRAMPercentage) ||
                            !FLAG_IS_DEFAULT(MinRAMFraction) ||
                            !FLAG_IS_DEFAULT(InitialRAMPercentage) ||
                            !FLAG_IS_DEFAULT(InitialRAMFraction)) &&
                            FLAG_IS_DEFAULT(MaxRAM);
   if (use_os_mem_limit) {
     phys_mem = os::physical_memory();
     FLAG_SET_ERGO(uint64_t, MaxRAM, (uint64_t)phys_mem);
   } else {
     phys_mem = FLAG_IS_DEFAULT(MaxRAM) ? MIN2(os::physical_memory(), 
(julong)MaxRAM)
                                        : (julong)MaxRAM;
   }

?

Thanks,
David
-----

> 
> Here’s a proposed CSR for this behavioral change.
> 
> https://bugs.openjdk.java.net/browse/JDK-8223957
> 
> 
> Bob.
> 
>> On May 14, 2019, at 9:26 AM, Thomas Schatzl <thomas.schatzl at oracle.com 
>> <mailto:thomas.schatzl at oracle.com>> wrote:
>>
>> Hi,
>>
>> On Mon, 2019-05-13 at 10:03 -0400, Bob Vandette wrote:
>>> Please review this suggested fix for correcting Heap size selection
>>> when -XX:{Max,Min,Initial)RAMPercentage and their Factional
>>> equivalent options are used.
>>>
>>> There are two bugs filed that are related to this issue (8222252 and
>>> 8213175)
>>>
>>> SUMMARY of FIX:
>>> The fix corrects what I believe is an incorrect implementation of the
>>> *Percentage/*Fraction options.  These options should not have caused
>>> the heap size to be fractionally based on MaxRAM or limited by
>>> MaxRAM.  They instead should be based on the host memory. This is
>>> what I assume was meant by “real memory”.
>>>
>>> product(double, MaxRAMPercentage,
>>> 25.0,                                   \
>>>         "Maximum percentage of real memory used for maximum heap
>>> size") \
>>>         range(0.0, 100.0)
>>>
>>> When these options are selected, they should take precedence over
>>> UseCompressedOops unless UseCompressedOops is also specified on the
>>> command line.
>>>
>>>
>>> WEBREV:
>>> http://cr.openjdk.java.net/~bobv/8222252/webrev.01
>>
>> - this is a style preference: I would assign the result of the
>> condition in the if (arguments.cpp:1721+) to the initialization of
>> use_os_mem_limit directly instead of first setting it to false, and
>> then to true again if needed. Feel free to ignore.
>>
>> - as David pointed out, this change needs a CSR.
>>
>> - the interaction between MaxRAM and MaxRAMPercentage is a bit unclear;
>> if they are independent as implemented now, then the update of one
>> should probably update the other. Or the VM should fail if they
>> disagree?
>>
>> If they are dependent, if David suggests (i.e. MaxRAMPercentage uses
>> MaxRAM as maximum real memory if set; this is what I would tend to
>> prefer), then not of course.
>>
>> - there should be regressions tests verifying this in cases where
>> possible.
>>
>> I.e. it should be doable to get current os::physical_memory  using e.g.
>> whitebox and then run a few tests verifying the results of MaxHeapSize
>> and coop mode.
>>
>> Particularly the interaction between MaxRAM, MaxRAMPercentage and coops
>> mode would be interesting.
>>
>> Thanks,
>>  Thomas
>>
>>
>