RFR: 8241825: Make compressed oops and compressed class pointers independent on x86_64
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu May 14 22:22:51 UTC 2020
On 5/14/20 2:04 PM, Erik Österlund wrote:
>
>
>> On 14 May 2020, at 22:20, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>
>> On 5/14/20 12:31 AM, Erik Österlund wrote:
>>> 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.
>>
>> Okay.
>>
>>>> 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.
>>
>> Good to know.
>>
>>>> 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.
>>
>> My concern is how class metaspace mapping will interact with Java heap mapping in this case. I assume (may be wrong) that both allocations will use HeapBaseMinAddress with your change. I always thought that for metaspace we will use somthing like HeapBaseMinAddress + java_heap_size address. May be I am totally wrong here. Please, explain.
>
> It will only try to put it in that low VA range if coop is disabled. Then the heap will be mapped in higher addresses. Sometimes on macOS though, the random address turns out to be quite low. Deferred fixing that to a separate RFE though that Thomas filed.
Okay.
>
>>
>>>> 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.
>>
>> okay
>>
>>>> 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.
>>
>> For +coop (shift 3) and +ccp(shift 3) cases we would get base == NULL (NULL+ptr<<3+heap_base) and execute code before this code.
>> Yes, we may execute this code for normal class pointer. Which is fine because make_ptr() which is called by get_ptr_type() handles both cases, narrow and normal pointers.
>>
>>> 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 fix though, but rather a way of having the JIT spend a few cycles less in some
>>> configurations. The then clause always works fine, but the else clause does not, but is faster to evaluate.
>>
>> I think we can simplify this code because make_ptr() can handle all cases and get_ptr_type() check for null in debug build as is_ptr() does.
>>
>> @@ -264,15 +263,8 @@
>> continue;
>> // cannot reason about it; is probably not implicit null exception
>> } else {
>> - const TypePtr* tptr;
>> - if (UseCompressedOops && (CompressedOops::shift() == 0 ||
>> - CompressedKlassPointers::shift() == 0)) {
>> - // 32-bits narrow oop can be the base of address expressions
>> - tptr = base->get_ptr_type();
>> - } else {
>> - // only regular oops are expected here
>> - tptr = base->bottom_type()->is_ptr();
>> - }
>> + // This handles both normal and compressed 32-bit pointer cases (when shift is 0).
>> + const TypePtr* tptr = base->get_ptr_type();
>> // Give up if offset is not a compile-time constant.
>> if (offset == Type::OffsetBot || tptr->_offset == Type::OffsetBot)
>> continue;
>
> That looks a lot simpler. Will file new RFE to clean that up.
Good.
Thanks,
Vladimir
>
> Thanks for your comments!
>
> /Erik
>
>> Thanks,
>> Vladimir
>>
>>> 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