RFR: 8213175 - MaxHeapSize refuses to go over 32178700288 when running in a docker container

[Bob Vandette]

Thanks for the feedback Thomas. 

You are correct that the change I implemented will cause us to select non coop mode
rather than cap the size of the heap.  This was not my intention.  This Heap Size flag processing
in Hotspot is non intuitive.  We make a decision to turn on UseCompressedOops based on the default
value of MaxHeapSize (96MB on x64) but then look at the real amount of memory in the system
and then assume we want to keep coops on rather than use a potentially much larger heap.
My implementation does all the heap sizing and then determines if we can enable CompressedOops
based on the size we determined.

The change that I had hoped to implement only disables coop mode if the user
specifies MaxRAMPercentage.  The justification is that if the user specified this
flag, they want this behavior over other conflicting ones.

I’ll try to refine my implementation to end up with a less invasive solution. 

See more comments below ...

> On Mar 14, 2019, at 7:19 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On Wed, 2019-03-13 at 08:35 -0400, Bob Vandette wrote:
>>> On Mar 13, 2019, at 12:40 AM, David Holmes <david.holmes at oracle.com
>>>> wrote:
>>> 
>>> Hi Bob,
>>> 
>>> On 13/03/2019 1:04 am, Bob Vandette wrote:
>>>> Please review this fix for correcting the Heap size when
>>>> MaxRAMPercentage is used.
>>>> BUG:
>>>> https://bugs.openjdk.java.net/browse/JDK-8213175
>>>> WEBREV:
>>>> http://cr.openjdk.java.net/~bobv/8213175/webrev.0/
>>>> SUMMARY:
>>>> This fix changes the ordering of when we try to determine if we
>>>> can enable UseCompressedOops.  It is now done after more of the
>>>> Heap size calculations are completed.
>>>> The only behavioral change should be that UseCompressedOops will
>>>> now be disabled if someone specifies a MaxRAMPercentage that
>>>> causes the heap size to exceed the amount of memory that can be
>>>> used with compressed oops.
>>>> Prior to this change, the amount of heap was reduced to the
>>>> compressed oop limit.
>>> 
>>> It isn't at all clear to me that this is the correct fix here. Why
>>> not assess this as "not an issue" and require the user to disable
>>> compressed oops?
>> 
>> The reason that I didn’t take that path is that I believe the new
>> behavior was the intention of the original author.
> 
> It would be really useful for this discussion if the old and/or the
> "intended behavior" were explained shortly, and the differences
> outlined (with pros/cons), and what happens when which flags are set.
> 
> At least to determine whether the new behavior is better and correctly
> implemented, the new behavior should be explained somewhere outside of
> the actual code change.
> 
> The (new) behavior must be verified using appropriate tests. A gigantic
> heap to trigger the code could be faked by using MaxRAM, and current
> CompressedOops mode can be determined eg. from -Xlog:gc+heap+coops
> output, so tests creating tests or extending existing ones (there are
> already some compressed oops jtreg tests afair) and should be fairly
> straightforward.

I believe there is a MaxRAMPercentage test.  I can add to that.

> 
> The reproducer-hard label in the CR should probably be removed.
> 

Ok.

>> The comment at line 1614 states that InitialHeapSize was the only
>> option that could impact MaxHeapSize
>> but the author was wrong.  The HeapRatio and now the HeapPercentage
>> options impact MaxHeapSize as well and weren’t taken into
>> consideration.
>> 
>> 1611 void Arguments::set_use_compressed_oops() {
>> 
>> 1612 #ifndef ZERO
>> 1613 #ifdef _LP64
>> 
>> 1614   // MaxHeapSize is not set up properly at this point, but
>> 1615   // the only value that can override MaxHeapSize if we are
>> 1616   // to use UseCompressedOops is InitialHeapSize.
>> 1617   size_t max_heap_size = MAX2(MaxHeapSize, InitialHeapSize);
>> 1618 
>> 
>>> 
>>> Such a behaviour change needs to be discussed and needs a CSR
>>> request.
>> 
>> Let’s see what the GC team wants me to do here.  
> 
> This is a significant externally visible change to behavior, so this
> needs a CSR regardless of pre-existing intended behavior or not.

I will file a CSR once I settle on an implementation.

> 
> The HeapPercentage CSR (JDK-8186315) does not indicate changes to the
> heap sizing ergonomics or compressed oop mode selection at all, so it
> does not cover this change.
> 
> With this change, on sufficiently large machines we now seem to select
> non-coop mode as default:

Correct, this is bad.  Will adjust algorithm.

> 
> E.g. old version:
> 
> $ java -Xlog:gc+heap+coops=trace -XX:MaxRAM=128g -version
> [0.007s][trace][gc,heap,coops] Trying to allocate at address
> 0x0000000082000000 heap of size 0x77e000000
> [0.018s][info ][gc,heap,coops] Heap address: 0x0000000082000000, size:
> 30688 MB, Compressed Oops mode: Zero based, Oop shift amount: 3
> openjdk version "10.0.2" 2018-07-17
> 
> (I do not think coops selection changed significantly since that
> version, but that's what I had at hand otherwise)
> 
> New:
> 
> $ java -XX:MaxRAM=128g -Xlog:gc+heap+coops=trace -version
> openjdk version "13-internal" 2019-09-17
> 
> I.e. no coops mode selected.
> 
> This RFR does not give performance numbers. Did you check for
> performance regressions in our benchmark suites?

Please send me a private email on how to run the benchmarks you
are concerned with.

> 
> If not, please do so, since we run many benchmarks without max heap
> size specification internally on machines with lots of memory (testing
> out-of-the-box behavior), this will likely cause differences everywhere
> (not to mention regressions) that need to be communicated and explained
> to the responsible persons preferably in advance.
> 
> (Please keep me in the loop, as I am also somewhat interested in this
> as I am the person that typically gets asked about this if the numbers
> went south….

Will do, Thanks.

Bob.

> )
> 

> Thanks,
>  Thomas
> 
>