RFR: JDK-8232069: Enable CDS even when UseCompressedClassPointers and/or UseCompressedOops are false

Tue Feb 11 17:08:37 UTC 2020

HI, Ioi,

    New updated URL with TestZGCWithCDS.java added.

http://cr.openjdk.java.net/~minqi/8232069/webrev-02/

    There is one failure but I don't think it is related to this change.

com/sun/jdi/InvokeHangTest.java

     I will double check and rerun the test.

     All passed hs-tier1-4 except for this failure.

Thanks

Yumin

On 2/10/20 10:52 AM, Yumin Qi wrote:
> Ioi,
>
>   I will add the ZGC test to the webrev and also remove it from 
> :hotspot_appcds_dynamic test group.
>
>   Will update when done.
>
>
> Thanks
>
> Yumin
>
> On 2/9/20 11:13 PM, Ioi Lam wrote:
>> Hi Yumin,
>>
>> Looks good.
>>
>> I noticed you used to have a test TestZGCWithCDS but it's no longer 
>> in the latest patch. I think it's a good idea to have a test that 
>> explicitly tests with ZGC.
>>
>> This test should also be excluded from the :hotspot_appcds_dynamic 
>> test group.
>>
>> Thanks
>> - Ioi
>>
>>
>> On 2/7/20 4:04 PM, Yumin Qi wrote:
>>> Hi, Calvin and Ioi
>>>
>>>   New updated webrev loaded at the same address:
>>> http://cr.openjdk.java.net/~minqi/8232069/webrev-01/ 
>>> <http://cr.openjdk.java.net/~minqi/8232069/webrev-01/>
>>>
>>>   There is one test failure in hs-tier3 for new added testcase: 
>>> runtime/cds/appcds/TestCombinedCompressedFlags.java
>>>
>>> The failure came from dynamic testing, since the flags stored in 
>>> default CDS, in dynamic dumping case, first will use the default 
>>> CDS, then compare the stored flags with runtime versions, if they do 
>>> not match, exit with newly added messages.
>>>
>>> The failure is not expected, since we expect the dump finished 
>>> successfully.
>>>
>>> The new change in TEST.groups excludes this test from 
>>> hotspot_appcds_dynamic.
>>>
>>>    Passed hs-tier1 - 4 tests.
>>>
>>> Thanks
>>>
>>> Yumin
>>>
>>>
>>> On 2/6/20 4:58 PM, Yumin Qi wrote:
>>>> HI, Calvin
>>>>
>>>>   Thanks for the review. Will update accordingly and test again.
>>>>
>>>>
>>>> Yumin
>>>>
>>>> On 2/6/20 4:06 PM, Calvin Cheung wrote:
>>>>> Hi Yumin,
>>>>>
>>>>> Changes look good overall. I have two comments:
>>>>>
>>>>> 1. Since you've added 2 fields in the archive header. I think the 
>>>>> CURRENT_CDS_ARCHIVE_VERSION should be updated in share/include/cds.h.
>>>>>
>>>>> 2. In the new test TestCombinedCompressedFlags.java:
>>>>>     Instead of:
>>>>>     171             System.out.println("Platform is not 64 bit, 
>>>>> skipped");
>>>>>     it is better to throw a SkippedException like the following:
>>>>>    throw new SkippedException("Platform is not 64 bit, skipped");
>>>>>    You would need to 'import jtreg.SkippedException'.
>>>>>
>>>>>    The following import statements seem unnecessary:
>>>>>
>>>>>       37 import jdk.test.lib.Asserts;
>>>>>       41 import jdk.test.lib.cds.CDSTestUtils;
>>>>>       42 import jdk.test.lib.process.ProcessTools;
>>>>>
>>>>>     Do you need the following @module?
>>>>>     33  *          java.management
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>> On 2/6/20 2:07 PM, Yumin Qi wrote:
>>>>>> Hi, Ioi and all
>>>>>>
>>>>>>
>>>>>>   Forget to mention tests: hs-tier1 - 4 passed.
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Yumin
>>>>>>
>>>>>> On 2/6/20 2:04 PM, Yumin Qi wrote:
>>>>>>> Hi, Ioi
>>>>>>>
>>>>>>>
>>>>>>>   Thanks for the review. See answers embedded.
>>>>>>>
>>>>>>>    New URL: http://cr.openjdk.java.net/~minqi/8232069/webrev-01/
>>>>>>>
>>>>>>> On 2/5/20 10:34 AM, Ioi Lam wrote:
>>>>>>>> Hi Yumin,
>>>>>>>>
>>>>>>>>
>>>>>>>> Looks good. Some nitpicks on the tests:
>>>>>>>>
>>>>>>>> CommandLineFlagComboNegative.java: the following line is not 
>>>>>>>> needed.
>>>>>>>>
>>>>>>>> import jdk.test.lib.cds.CDSTestUtils.Result;
>>>>>>>>
>>>>>>> Removed.
>>>>>>>>
>>>>>>>> TestCombinedCompressedFlags.java: DUMP_NORMAL_MSG doesn't seem 
>>>>>>>> to be used by the test.
>>>>>>>>
>>>>>>> Removed.
>>>>>>>
>>>>>>> Also changed to use TestCommon.dump/TestCommon.exec which better 
>>>>>>> to check dum/run results.
>>>>>>>
>>>>>>>> DifferentHeapSizes.java: after your change, 
>>>>>>>> CDSTestUtils.MSG_COMPRESSION_MUST_BE_USED is no longer used 
>>>>>>>> anywhere in the tests. Also, This message is no longer printed 
>>>>>>>> by the VM. I think the 
>>>>>>>> CDSTestUtils.MSG_COMPRESSION_MUST_BE_USED field should be removed.
>>>>>>>>
>>>>>>>>
>>>>>>> Removed.
>>>>>>>> IncompatibleOptions.java: COOPS_DUMP_WARNING can be removed.
>>>>>>>>
>>>>>>> Removed.
>>>>>>>> The check_shared parameter for testExec() should be removed. 
>>>>>>>> It's currently used only when expectedToFail == false. However, 
>>>>>>>> in all the cases where expectedToFail == false, check_shared is 
>>>>>>>> always true.
>>>>>>>>
>>>>>>>>
>>>>>>> Removed.
>>>>>>>
>>>>>>>> Also, in filemap.cpp, before checking "compressed_oops() != 
>>>>>>>> UseCompressedOops ...", I think it will be good to add a log to 
>>>>>>>> print out the expected compressed mode. That way if the user 
>>>>>>>> gets an error, they can run with -Xlog:cds to find out exactly 
>>>>>>>> what the reason is. E.g.
>>>>>>>>
>>>>>>>>     log_info(cds)("Archive was created with UseCompressedOops = 
>>>>>>>> %d, UseCompressedClassPointers = %d",
>>>>>>>>                           compressed_oops(), 
>>>>>>>> compressed_class_pointers());
>>>>>>>>     if (compressed_oops() != UseCompressedOops ...
>>>>>>>>
>>>>>>> Added.
>>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Yumin
>>>>>>>
>>>>>>>>
>>>>>>>> On 2/4/20 6:01 PM, Yumin Qi wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>>    Please review fix:
>>>>>>>>>
>>>>>>>>>    Bug: https://bugs.openjdk.java.net/browse/JDK-8232069
>>>>>>>>>
>>>>>>>>> Webrev:http://cr.openjdk.java.net/~minqi/8232069/webrev/
>>>>>>>>>
>>>>>>>>>     Description: Currently CDS only works when using 
>>>>>>>>> compressed Oops turned on, -XX:+UseCompressedOops (default), 
>>>>>>>>> the fix is allowing CDS works with uncompressed oops and 
>>>>>>>>> uncompressed class pointers. The contents of 
>>>>>>>>> InstanceKlass::_fields are depending on UseCompressedOops and 
>>>>>>>>> UseCompressedClassPointers on/off. There are three suggestions 
>>>>>>>>> as described in bug description:
>>>>>>>>>
>>>>>>>>> 1. Remember the compression mode at CDS dump time. At runtime, 
>>>>>>>>> disable CDS if the compression mode is changed.
>>>>>>>>> 2. Same as 1, but save more than one CDS archives in the JDK 
>>>>>>>>> image. Choose the appropriate one at start-up
>>>>>>>>> 3. Recompute re-compute InstanceKlass::_fields at runtime.
>>>>>>>>>
>>>>>>>>> The fix takes the first one, storing UseCompressedOops and 
>>>>>>>>> UseCompressedClassPointers in shared archive. CDS is only 
>>>>>>>>> allowed at runtime if the flags are consistent with the stored 
>>>>>>>>> versions in archive.
>>>>>>>>>
>>>>>>>>>  Testing: hs-tier1-2
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>   Thanks
>>>>>>>>>
>>>>>>>>>   Yumin
>>>>>>>>>
>>>>>>>>
>>