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

[Calvin Cheung]

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