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

[Per Liden]

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

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