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

Wed May 13 18:44:16 UTC 2020

Hi Erik

x86 changes looks fine to me (assembler, C1, C2).

How src/hotspot/share/classfile/* changes are related to this?

I don't get changes in metaspace.cpp. Why use HeapBaseMinAddress when COOP is off (looks like you want klass heap at the 
beginning of Javva heap)? Can you add comment?

arguments.cpp - should you also check UseNewFieldLayout flag as you do in instanceOop.hpp?

lcm.cpp - I concern about this change. This code is for compressed oops only - implicit null check is for oops.

Thanks,
Vladimir

On 5/12/20 7:09 AM, Erik Österlund wrote:
> 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
> 