RFR: 8241825: Make compressed oops and compressed class pointers independent on x86_64
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed May 13 19:35:16 UTC 2020
At least lcm.cpp changes should be something like next because compressed oops and classes are independent now:
if ((UseCompressedOops && CompressedOops::shift() == 0) ||
(UseCompressedClassPointers && CompressedKlassPointers::shift() == 0)) {
Vladimir
On 5/13/20 11:44 AM, Vladimir Kozlov wrote:
> 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
>>
More information about the hotspot-dev
mailing list