RFR: 8241825: Make compressed oops and compressed class pointers independent on x86_64
Erik Österlund
erik.osterlund at oracle.com
Wed May 13 13:45:14 UTC 2020
Hi Thomas,
Excellent, thanks for filing that Thomas.
Thanks,
/Erik
On 2020-05-13 15:39, Thomas Stüfe wrote:
> 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 <mailto: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 <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