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

Wed May 13 05:40:07 UTC 2020

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.

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
>
>