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

Wed Apr 29 06:27:31 UTC 2020

Hi Erik,

On Tue, Apr 28, 2020 at 6:28 PM Erik Österlund <erik.osterlund at oracle.com>
wrote:

> Hi Thomas,
>
> Thanks for reviewing this.
>
> On 2020-04-28 13:14, Thomas Stüfe wrote:
>
> Hi Erik,
>
> On Tue, Apr 28, 2020 at 10:07 AM Erik Österlund <erik.osterlund at oracle.com>
> wrote:
>
>> Hi Thomas,
>>
>> On 2020-04-28 08:43, Thomas Stüfe wrote:
>>
>> Hi Erik,
>>
>> nice change, and now I do not have to keep in mind "heap>32G = no class
>> space" :)
>>
>>
>> Glad you liked it!
>>
>> I eyeballed the metaspace changes (what little there are).
>>
>> This:
>>
>> -     char* base = (char*)align_up(CompressedOops::end(), _reserve_alignment);+     char* base;+     if (UseCompressedOops) {+       base = (char*)align_up(CompressedOops::end(), _reserve_alignment);+     } else {+       base = (char*)HeapBaseMinAddress;+     }
>>
>> may not work as intended. You probably want the end of the reserved heap as attach point for ccs, not the HeapBaseMinAddress.
>>
>>
>> Could you please expand on that? To make it clear, what this code does is
>> to put metaspace at the
>> minimum address we are allowed to map, when compressed oops is disabled.
>> The reasoning for doing
>> that is that if the low address bits (resulting in good compressed
>> pointers) are not used by compressed oops,
>> then they are available to be used by compressed class pointers instead,
>> such that at least compressed class
>> pointers can utilize this VA to get optimal encoding of class pointers.
>>
>> Is there anything not desirable about this? If so, I can't see it, and
>> would appreciate if you expand on why
>> that would be a bad idea. Perhaps there is something I do not see here.
>>
>>
> Oh, this was a long standing confusion in my head. I never noticed that
> HeapBaseMinAddress exists for the sole purpose of compressed oops, and is
> ignored otherwise. So I thought what you do there is just mapping ccs over
> the start of where the user wants the heap to start, and since that may not
> work we would fall back to map-anywhere.
>
> So you are saying HeapBaseMinAddress is the lowest address the platform
> allows us to map to, irregardless of whether we run with compressed oops or
> not.
>
>
> Right, so the purpose of this flag was to be able to move the heap up,
> because if we map things too low, we ran into conflicts with sbrk, and
> hence some old suboptimal malloc implementations, such as the default
> malloc on Solaris. This called for a flag to be able to move the heap up,
> putting it out of the way from sbrk. That is how HeapBaseMinAddress came
> about, AFAIK. The situation when mapping in metaspace and compressed class
> space, without compressed oops, is very much the same: we want to go as low
> as possible for optimal encoding, but not too low so we run into these
> problems.
>
> 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.

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!

..Thomas

> Thanks,
> /Erik
>
> I will have a look at that for the next webrev.
>>
>> Thanks for the review.
>>
>> /Erik
>>
>>
>
> Thanks,
>
> Thomas
>
>
>
>> Thank you, Thomas
>>
>>
>> On Fri, Apr 24, 2020 at 10:19 AM Erik Österlund <
>> erik.osterlund at oracle.com> wrote:
>>
>>> Hi,
>>>
>>> Today, the use of compressed class pointers assumes the use of
>>> compressed oops.
>>> This patch loosens up the relationship between compressed oops and
>>> compressed
>>> class pointers, so that compressed class pointers can be used without
>>> compressed
>>> oops. This benefits for example ZGC that wants compressed class
>>> pointers, but
>>> not compressed oops, effectively giving ZGC 4 bytes smaller objects.
>>>
>>> Much of the complexity of the change is that r12 used to be used by
>>> compressed
>>> class pointers as some kind of semi-fast temp register that one would
>>> restore
>>> to the compressed oops heap base (or zero). That made it effectively a
>>> slightly
>>> optimized spilling mechanism used to find a temp register. I replaced
>>> that
>>> mechanism with a plain old normal temp register that you pass in as a
>>> parameter.
>>> The bad news is that I had to find available temp registers in a lot of
>>> places.
>>> The good news is that almost always were there temp registers available
>>> for free,
>>> and hence the new temp register is even faster than the old optimized
>>> spilling
>>> mechanism. Because we almost never need any spilling at all in the
>>> contexts where
>>> this is used.
>>>
>>> Since I want the 4 new bytes to actually make objects smaller, I poked
>>> the new
>>> layout code a bit so that the klass gap is made available for fields.
>>> That used
>>> to be made available only with compressed oops enabled, due to
>>> restrictions in
>>> the layout code. Our new layout code does not have such restrictions,
>>> and so
>>> I will make the 4 bytes available for fields when the new layout code is
>>> used
>>> and compressed class pointers are used.
>>>
>>> Now I'm only fixing this for HotSpot x86_64. Ideally the use of
>>> compressed oops
>>> and compressed class pointers should not be entangled in other platforms
>>> and
>>> Graal. But that is beyond the scope of this change, and I will let them
>>> behave
>>> the way that they used to, to be potentially fixed later.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8241825
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8241825/webrev.00/
>>>
>>> Testing:
>>> hs-tier1-7
>>>
>>> Thanks,
>>> /Erik
>>>
>>
>>
>