RFR: 8241825: Make compressed oops and compressed class pointers independent on x86_64
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu May 14 20:19:58 UTC 2020
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.
>
>> 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;
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