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

[Per Liden]

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