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