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

Wed May 13 13:39:04 UTC 2020

Hi Erik,

I filed https://bugs.openjdk.java.net/browse/JDK-8244943 as a follow up for
the improved ccs placement. But lets get the cds remodel go through first...

Cheers, Thomas

On Wed, May 13, 2020 at 11:13 AM Erik Österlund <erik.osterlund at oracle.com>
wrote:

> Hi Thomas,
>
> On 2020-05-13 07:42, Thomas Stüfe wrote:
>
> p.s. I also think that we can rework this test and ccs placement in
> general at a later follow up rfe. ccs placement strategy is not optimal but
> that would be out of scope for both your change and my cds rework patch,
> which is supposed to be mainly code shuffling around.
>
>
> Yes, agreed.
>
> ..Thomas
>
> On Wed, May 13, 2020 at 7:40 AM Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
>
>> Hi Erik,
>>
>> note that I am changing the CompressClassPointers too as part of
>> "8243392: Remodel CDS/Metaspace storage reservation" which is out for
>> review. That is not a big deal, if you push before me (which I assume will
>> happen) I'll merge. But look at my changes:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev.02/webrev/test/hotspot/jtreg/runtime/CompressedOops/CompressedClassPointers.java.udiff.html
>>
>> What I did was to explicitly disable placement checks on selected
>> platforms - windows and, for us, ppc. So the tests run in two modes, one
>> only testing that the VM does come up with the given parameters, one more
>> strict where we test ccs placement. Feel free to integrate that into your
>> change.
>>
>> However, that said, I am fine with your change as it is. I would merge
>> your changes into my change then.
>>
>
> Ah, okay. Thanks for the pointer. Then I shall push very soon. Thank you
> for dealing with the merge Thomas. And thanks for your review and helpful
> comments.
>
> /Erik
>
> Cheers Thomas
>>
>>
>>
>> On Tue, May 12, 2020 at 4:10 PM Erik Österlund <erik.osterlund at oracle.com>
>> 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
>>>
>>>
>