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

[Ioi Lam]

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