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

Tue Feb 11 17:21:22 UTC 2020

Ioi,

    Thanks for re-re-review!

Yumin

On 2/11/20 9:17 AM, Ioi Lam wrote:
> Looks good!
>
> Thanks
> - Ioi
>
> On 2/11/20 9:08 AM, Yumin Qi wrote:
>>
>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>
>