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

Tue May 12 14:09:47 UTC 2020

Hi,

I ran into some merge conflicts in the tests and the new tests behaved a 
bit differently
on different platforms. The new tests naturally do not behave the same 
way on the platforms that do
not yet support compressed class pointers without compressed oops, so I 
just filtered away those
platforms from running the new tests. Will update the filter when they 
start supporting it.

Another interesting thing is that on macOS, sometimes we did not get the 
expected compressed class
pointer encoding without compressed oops. The reason is that without 
compressed oops, we allocate
the heap at some random address where the OS thinks is suitable. 
Sometimes that is where we would
ideally have put the compressed class space. This forces metaspace to be 
mapped in somewhere else
with different characteristics. Ideally, the bootstrapping logic should 
be disentangled so that
when you run without compressed oops, metaspace gets reserved before the 
heap. But I think that
warrants a follow-up RFE, as I do not want to complicate this patch any 
further. Disentangling that
seems like it could become quite tricky on its own.

So here is another webrev with the test issues resolved:
http://cr.openjdk.java.net/~eosterlund/8241825/webrev.02/

Incremental:
http://cr.openjdk.java.net/~eosterlund/8241825/webrev.01..02/

Thanks,
/Erik

On 2020-04-29 09:39, Erik Österlund wrote:
> 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