RFR: 8241825: Make compressed oops and compressed class pointers independent on x86_64

[Erik Österlund]

Hi Thomas,

On 2020-04-29 08:27, Thomas Stüfe wrote:
> Hi Erik,
>
> On Tue, Apr 28, 2020 at 6:28 PM Erik Österlund 
> <erik.osterlund at oracle.com <mailto:erik.osterlund at oracle.com>> wrote:
>
>     Hi Thomas,
>
>     Thanks for reviewing this.
>
>     On 2020-04-28 13:14, Thomas Stüfe wrote:
>
>>     With compressed oops we continue to squeeze ccs in after the heap
>>     in the hope that it all still fits into the lower 4G or 32G. (Do
>>     we ever bother to check if heap is actually reserved in low area?
>>     It may have given up and reserved upstate instead, leaving free
>>     room below...).
>
>     Right. And yes I'm pretty sure we do check. The heap reserves
>     memory, and sets the CompressedOops::end() based on that, which is
>     fed as input to the metaspace/CDS placement.
>
>
> No, what I meant was: since we reserve at CompressedOops::end(), if 
> the heap was too large to place it in the lower address regions, the 
> class space will not live there either even if it could, since it is 
> smaller and might fit.
>
> For example:
>
> thomas at mainframe:/shared/projects/openjdk/jdk-jdk/source$ 
> ../../jdks/sapmachine14/bin/java -Xlog:gc+metaspace=trace -Xshare:off 
> -Xmx31g
> [0.003s][trace][gc,metaspace] node @0x00007fca441b9f90: 
> reserved=1048576.00 KB, committed=0.00 KB (  0%), used=0.00 KB (  0%)
> [0.003s][trace][gc,metaspace]  [0x00000017c0400000, 
> 0x00000017c0400000, 0x00000017c0400000, 0x0000001800400000)
> [0.003s][trace][gc,metaspace] Narrow klass base: 0x00000017c0400000, 
> Narrow klass shift: 0
> [0.003s][trace][gc,metaspace] Compressed class space size: 1073741824 
> Address: 0x00000017c0400000 Req Addr: 0x00000017c0400000
> [0.003s][trace][gc,metaspace] node @0x00007fca441ba190: 
> reserved=8192.00 KB, committed=0.00 KB (  0%), used=0.00 KB (  0%)
> [0.003s][trace][gc,metaspace]  [0x00007fc9cb800000, 
> 0x00007fc9cb800000, 0x00007fc9cb800000, 0x00007fc9cc000000)
>
> (large heap and no CDS, since it prevents zero based mode).
>
> In that case narrow klass base is not zero based even it if probably 
> could be, if we were to allocate class space in the lower regions, 
> which are now unoccupied since the large heap does not fit there. A 
> missed opportunity to go zero based. Not a big deal, since we mostly 
> run with CDS now anyway.

I see. Yeah, there is room for improvement there. Although, as you say, 
people mostly run with CDS on, so it probably won't matter even if that 
is improved.

>>     Now with your patch without compressed oops we attempt to map at
>>     HeapBaseMinAddress since this is the lowest address we are
>>     allowed to map at and the heap won't obviously need it, right?
>
>     Exactly.
>
>>     My only misgiving would be that HeapBaseMinAddress is even more
>>     of a misnomer (its already now a bit, witness my year long
>>     confusion :-), since it now controls not only heap placement but
>>     ccs too. Actually if it is that important to avoid mapping to
>>     addresses below HeapBaseMinAddress we should probably prevent it
>>     somewhere below in the os layer but I really do not want to open
>>     that can of worms.
>
>     Yes agreed. Ideally it would be called something like
>     MappingMinAddress instead, because it really isn't tied to the
>     heap really, it just happened to be the primary user of mapping
>     things at low addresses. But I also think changing that, and
>     enforcing it more widely in the os layer sounds like a different
>     can of worms.
>
>>     --
>>
>>     Some more remarks about arguments.cpp:
>>
>>     - In Arguments::set_use_compressed_klass_ptrs(), if now
>>     compressed class pointers are independent from compressed oops,
>>     checking CompressedClassSpaceSize for validity should now always
>>     happen for UseCompressedClassPointers=1.
>
>     I think it looks like I do that.
>
>>     - If HeapBaseMinAddress is now always used, we should check it
>>     always too (see Arguments::set_heap_size)
>
>     Good idea. Fixed.
>
>>
>>>         Also, could we have some tests which exercise cds/metaspace
>>>         initialization without coops and with ccps? Easiest way
>>>         would be to extend
>>>         "CompressedOops/CompressedClassPointers.java" by some cases.
>>
>>
>>     Thank you. I think this makes sense especially since compressed
>>     class pointer encoding is restricted to one mode if CDS is on
>>     (base = shared address and shift=3), and since CDS=on is the
>>     default zerobased encoding may not happen that often.
>
>     Yup. I added some explicit test cases for -XX:-UseCompressedOops
>     -XX:+UseCompressedClassPointers as suggested. This will also be
>     the default configuration for ZGC, so that should give that
>     configuration more test coverage in the various tests we have.
>
>     Here is a new webrev:
>     http://cr.openjdk.java.net/~eosterlund/8241825/webrev.01/
>
>     Incremental:
>     http://cr.openjdk.java.net/~eosterlund/8241825/webrev.00_01/
>
>     I hope I fixed everything you wanted in this patch. I also fixed
>     what Frederic and Coleen wanted.
>
>
> arguments.cpp, the metaspace stuff and the test look good to me now. 
> Thank you for your work!

Thanks for the review Thomas!

/Erik