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

[Calvin Cheung]

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