RFR (XS) 8205702: assert(UseCompressedClassPointers) failed in universe.hpp

[Per Liden]

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