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

[Yumin Qi]

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