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

[Thomas Schatzl]

Hi,

  one addition: the change does not build on OSX and Solaris.

Arguments::set_heap_size() the MAX2() in the first
set_use_compressed_oops() call has differing types there  -
reasonable_max is ulonglong, while InitialHeapSize is ulong.

The second use_compressed_oops() call in the same method should be
aligned properly.

Please fix.

Thanks,
  Thomas

On Thu, 2019-03-14 at 12:19 +0100, Thomas Schatzl 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.
> 
> The reproducer-hard label in the CR should probably be removed.
> 
> > 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.
> 
> 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:
> 
> 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?
> 
> 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....)
> 
> Thanks,
>   Thomas
> 
>