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

[Yumin Qi]

Hi, Calvin

   Thanks for the final review.


Yumin

On 2/10/20 9:23 AM, Calvin Cheung wrote:
>
> Hi Yumin,
>
> The updated webrev looks good.
>
> thanks,
> Calvin
> 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
>>>>>>>>
>>>>>>>