[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-gc-dev
Subject: Re: RFR (XS) 8205702: assert(UseCompressedClassPointers) failed in universe.hpp
From: Per Liden <per.liden () oracle ! com>
Date: 2018-06-27 21:51:45
Message-ID: 435d050f-c25e-bdc0-235b-ed85bf65c095 () oracle ! com
[Download RAW message or body]
Thanks Calvin!
/Per
On 06/27/2018 11:49 PM, Calvin Cheung wrote:
> Looks good.
>
> thanks,
> Calvin
>
> On 6/27/18, 2:22 PM, Per Liden wrote:
>> Sorry, but I noticed that the IncompatibleOptions.java assumes that
>> ZGC is always available, but it's only available on linux-x64, so this
>> will fail on other platforms. Updated webrev to take this into
>> account. It's currently running in mach5 t{1-3} on all platforms.
>>
>> http://cr.openjdk.java.net/~pliden/8205702/webrev.4
>>
>> /Per
>>
>> On 06/27/2018 08:34 PM, Per Liden wrote:
>>> On 06/27/2018 08:29 PM, Calvin Cheung wrote:
>>>>
>>>>
>>>> On 6/27/18, 11:08 AM, Per Liden wrote:
>>>>> Hi Calvin,
>>>>>
>>>>> On 06/27/2018 07:39 PM, Calvin Cheung wrote:
>>>>>> Hi Per,
>>>>>>
>>>>>> Thanks for coming up with a simpler fix. It looks good. Just one
>>>>>> comment below.
>>>>>>
>>>>>> On 6/27/18, 2:33 AM, Per Liden wrote:
>>>>>>> Actually, that seems a bit too restrictive as
>>>>>>> vm.cds.archived.java.heap is only true when G1 is enabled.
>>>>>>>
>>>>>>> So, this is probably even better:
>>>>>>>
>>>>>>> * @requires vm.cds
>>>>>>> * @requires vm.opt.final.UseCompressedOops
>>>>>>> * @requires vm.opt.final.UseCompressedClassPointers
>>>>>> I think the @requires vm.cds calls into VMProps.vmCDS() which
>>>>>> calls the WB_IsCDSIncludedInVmBuild() where it already checks for
>>>>>> compressed oops and pointers:
>>>>>>
>>>>>> if (!UseCompressedOops || !UseCompressedClassPointers) {
>>>>>> // On 64-bit VMs, CDS is supported only with compressed
>>>>>> oops/pointers
>>>>>> return false;
>>>>>> }
>>>>>>
>>>>>> Are the last two @requires needed?
>>>>>
>>>>> That's an excellent point, and you're right, those extra @requires
>>>>> are not needed. New webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~pliden/8205702/webrev.3
>>>> Looks good.
>>>
>>> Thanks Calvin!
>>>
>>> /Per
>>>
>>>>
>>>> thanks,
>>>> Calvin
>>>>>
>>>>> Thanks for reviewing!
>>>>>
>>>>> cheers,
>>>>> Per
>>>>>
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>>>
>>>>>>> Updated webrev: http://cr.openjdk.java.net/~pliden/8205702/webrev.2
>>>>>>>
>>>>>>> /Per
>>>>>>>
>>>>>>> On 06/27/2018 10:37 AM, Per Liden wrote:
>>>>>>>> Updated webrev, which adjusts the @requires tag, from:
>>>>>>>>
>>>>>>>> @requires vm.cds & vm.gc != "Z"
>>>>>>>>
>>>>>>>> to:
>>>>>>>>
>>>>>>>> @requires vm.cds.archived.java.heap
>>>>>>>>
>>>>>>>> which I believe is more correct in this case.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~pliden/8205702/webrev.1
>>>>>>>>
>>>>>>>> cheers,
>>>>>>>> Per
>>>>>>>>
>>>>>>>>
>>>>>>>> On 06/27/2018 09:15 AM, Per Liden wrote:
>>>>>>>>> Hi Coleen,
>>>>>>>>>
>>>>>>>>> This doesn't look quite right to me. ZGC already disables
>>>>>>>>> UseCompressedOop and UseCompressedClassPointers, which should
>>>>>>>>> be the indicators that we can't use CDS. The problem is that
>>>>>>>>> CDS checks those flags _before_ the heap has had a change to
>>>>>>>>> say they it supports. So if we just move the call to
>>>>>>>>> set_shared_spaces_flags() after the call to
>>>>>>>>> GCConfig::arguments()->initialize() (which should be safe),
>>>>>>>>> then we're all good and you'll get the usual:
>>>>>>>>>
>>>>>>>>> $ ./build/fastdebug/images/jdk/bin/java -Xshare:dump
>>>>>>>>> -XX:+UnlockExperimentalVMOptions -XX:+UseZGC
>>>>>>>>> Error occurred during initialization of VM
>>>>>>>>> Cannot dump shared archive when UseCompressedOops or
>>>>>>>>> UseCompressedClassPointers is off.
>>>>>>>>>
>>>>>>>>> Here's a proposed patch for this, which also adjusts the
>>>>>>>>> appropriate tests for this:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~pliden/8205702/webrev.0
>>>>>>>>>
>>>>>>>>> cheers,
>>>>>>>>> Per
>>>>>>>>>
>>>>>>>>> On 06/27/2018 01:09 AM, coleen.phillimore@oracle.com wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Calvin, thank you for reporting the bug and the code review
>>>>>>>>>> and test code.
>>>>>>>>>>
>>>>>>>>>> On 6/26/18 5:42 PM, Calvin Cheung wrote:
>>>>>>>>>>> Hi Coleen,
>>>>>>>>>>>
>>>>>>>>>>> The code changes look good.
>>>>>>>>>>>
>>>>>>>>>>> Since there's a new error message, I'd suggest adding a test
>>>>>>>>>>> to runtime/SharedArchiveFile/SharedArchiveFile.java as follows:
>>>>>>>>>>>
>>>>>>>>>>> diff --git
>>>>>>>>>>> a/test/hotspot/jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java
>>>>>>>>>>> b/test/hotspot/jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>> a/test/hotspot/jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java
>>>>>>>>>>>
>>>>>>>>>>> +++
>>>>>>>>>>> b/test/hotspot/jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java
>>>>>>>>>>>
>>>>>>>>>>> @@ -52,5 +52,13 @@
>>>>>>>>>>> "-Xshare:on", "-version");
>>>>>>>>>>> out = CDSTestUtils.executeAndLog(pb,
>>>>>>>>>>> "SharedArchiveFile");
>>>>>>>>>>> CDSTestUtils.checkExec(out);
>>>>>>>>>>> +
>>>>>>>>>>> + // CDS dumping doesn't work with ZGC
>>>>>>>>>>> + ProcessBuilder pb =
>>>>>>>>>>> ProcessTools.createJavaProcessBuilder(true,
>>>>>>>>>>> + "-XX:SharedArchiveFile=./SharedArchiveFile.jsa",
>>>>>>>>>>> + "-XX:+UseZGC",
>>>>>>>>>>> + "-Xshare:dump");
>>>>>>>>>>> + out = CDSTestUtils.executeAndLog(pb,
>>>>>>>>>>> "SharedArchiveFile");
>>>>>>>>>>> + CDSTestUtils.checkExecExpectError(out, 1,
>>>>>>>>>>> "DumpSharedSpaces (-Xshare:dump) is not supported with ZGC.");
>>>>>>>>>>> }
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> (I haven't tested the above)
>>>>>>>>>>
>>>>>>>>>> It needed an -XX:+UnlockExperimentalVMOptions as well, and not
>>>>>>>>>> reclare pb.
>>>>>>>>>>
>>>>>>>>>> open webrev at
>>>>>>>>>> http://cr.openjdk.java.net/~coleenp/8205702.02/webrev
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Also, I think the new error message should be included in the
>>>>>>>>>>> release notes.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I added the test case and it passes. I don't think having a
>>>>>>>>>> release note for something that nobody would ever do for an
>>>>>>>>>> experimental option is worth having. But I can look into the
>>>>>>>>>> ZGC release notes and see if there's something that says CDS
>>>>>>>>>> is not supported.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Coleen
>>>>>>>>>>> thanks,
>>>>>>>>>>> Calvin
>>>>>>>>>>>
>>>>>>>>>>> On 6/26/18, 2:13 PM, coleen.phillimore@oracle.com wrote:
>>>>>>>>>>>> Summary: Disable CDS with ZGC
>>>>>>>>>>>>
>>>>>>>>>>>> Tested with:
>>>>>>>>>>>> java -XX:+UnlockExperimentalVMOptions -XX:+UseZGC -Xshare:dump
>>>>>>>>>>>> java -XX:+UnlockExperimentalOptions -XX:+UseZGC -Xshare:on
>>>>>>>>>>>> -version
>>>>>>>>>>>>
>>>>>>>>>>>> open webrev at
>>>>>>>>>>>> http://cr.openjdk.java.net/~coleenp/8205702.01/webrev
>>>>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8205702
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Coleen
>>>>>>>>>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic