RFR: 8241825: Make compressed oops and compressed class pointers independent on x86_64
Erik Österlund
erik.osterlund at oracle.com
Wed May 13 09:13:23 UTC 2020
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
> <mailto: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 <mailto: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><mailto: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