RFR: 8241825: Make compressed oops and compressed class pointers independent on x86_64
Erik Österlund
erik.osterlund at oracle.com
Thu May 14 07:31:49 UTC 2020
Hi Kozlov,
Thanks for the review. I pushed this before the review though, so any
comments I will deal with in a follow-up RFR.
Let'swork out first what needs changing.
On 2020-05-13 20:44, Vladimir Kozlov wrote:
> Hi Erik
>
> x86 changes looks fine to me (assembler, C1, C2).
Thanks.
> How src/hotspot/share/classfile/* changes are related to this?
Just enabling -coop +ccp gives us back 4 bytes in the klass gap. But the
layout engine for InstanceKlass
doesn't putany fields there, it aligns up to the next word by default.
In other words, a lot of changes for
almostno gain (except arrays that utilize this), unless the klass gap is
given back to the layout code in this
configuration. So I gave the klass gap to the fieldlayout code, which
caused some fan out. Those files are
the result of that.
> 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?
I can add a comment. It's because HeapBaseMinAddress is the JVM switch
for the lowest address we can map
in memory without getting dangerously close to bad sbrk interactions,
e.g. due to poor malloc implementations.
Had a conversation with Thomas about this, and the conclusion is that
the name of the flag is unfortunate
but it is the value I should be using.
> arguments.cpp - should you also check UseNewFieldLayout flag as you do
> in instanceOop.hpp?
That is not necessary. The -coop +ccp combination with
-XX:-UseNewFieldLayout still works fine, it just happens
to be that it is unable to fully benefit from the fact that class
pointers are being used for instanceOops, but
it still does benefit for arrays, which is not as great, but not wrong.
> lcm.cpp - I concern about this change. This code is for compressed
> oops only - implicit null check is for oops.
Right. I wasn't quite sure about this one. It seemed like the if
statement captured the condition that there *could*
be a compressed pointer without shift, but could also include cases
where this is impossible, i.e. allowing false
positives (but not false negatives, crucial for correctness). For
example, if you previously ran with
+coop (shift 3) and -ccp (shift 0), then this if would take the "then"
path, despite it being impossible for a
compressed pointer with shift 0 being present. So I continued to model
it as having false positives but importantly
no false negatives, like before.
Having said that, perhaps we could increase the accuracy of that check,
getting rid of false negatives. I don't
believe that is a correctness fixthough, but rather a way of having the
JIT spend a few cycles less in some
configurations. The then clausealways works fine, but the else clause
does not, but is faster to evaluate.
Thanks,
/Erik
> 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