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

Tue Feb 11 17:17:38 UTC 2020

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