RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Ioi Lam]

Hi Jiangli,

The code changes look good to me. I agree with David's comments about 
the test cases.

Thanks

- Ioi


On 4/24/18 11:17 PM, David Holmes wrote:
> cc'ing build-dev for the makefile change
>
> Hi Jiangli,
>
> On 25/04/2018 10:53 AM, Jiangli Zhou wrote:
>> Please review the following changes that streamline the support for 
>> application class data sharing by eliminating the requirement of 
>> using -XX:+UseAppCDS to enable the feature. The support for 
>> application class data sharing is now enabled transparently when 
>> application classes or platform classes present in the classlist or 
>> generated archive with -Xshare:dump/on/auto.
>>
>>     RFE: https://bugs.openjdk.java.net/browse/JDK-8193213
>>     Bug: https://bugs.openjdk.java.net/browse/JDK-8182731
>
> These issues are not publicly visible, can you change them to be so?
>
>>     webrev: 
>> http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev.00/
>
> Overall this seems fine. Thanks for explaining the logic changes below 
> - much appreciated!
>
> One query: why was UseAppCDS not removed from the tests (or at least 
> the tests you were modifying anyway) ? They will all need updating 
> before 12 when the flag is expired.
>
> test/hotspot/jtreg/runtime/appcds/SharedArchiveFile.java
>
> Test 2 reference to UseAppCDS seems unnecessary now.
> Test 3 is not needed as you're not using any diagnostic flag now.
> Test 4 seems unnecessary now.
>
> test/hotspot/jtreg/runtime/appcds/TestCommon.java
>
> makeCommandLineForAppCDS should just be removed (yes that will cause 
> some churn in the other tests) - but this can be a follow up test 
> cleanup.
>
> test/hotspot/jtreg/runtime/appcds/UseAppCDS.java
>
> This test no longer makes much sense to me. The only tests should be 
> that app classes get archived and used. It makes no sense to claim to 
> be testing -XX:+UseAppCDS when that flag is obsolete. Likewise now 
> that it is obsolete you don't need to test that -XX:-UseAppCDS has no 
> affect.
>
> Thanks,
> David
>
>
>> Highlights of the changes:
>> * UseAppCDS is obsolete in JDK 11
>> * Remove the +UnlockCommercialFeatures requirement when using 
>> application class data sharing in OracleJDK binary
>> * SharedArchiveFile is a product flag for all configurations in JDK 11
>> * DumpLoadedClassList produces a complete classlist including all 
>> loaded library and application classes
>>
>> Thanks David Holmes for his guidance on CSR process.
>>
>> Following are the details for the VM and test changes:
>> - Removed various ‘if (UseAppCDS)’ checks and UseAppCDS asserts.
>>
>> - Removed some of the CDS/AppCDS specifics from general class loading 
>> code.
>>
>> - Removed scattered checks for non-empty directory. Dump time 
>> non-empty directory check is done at one central place, 
>> FileMapInfo::check_nonempty_dir_in_shared_path_table() after loading 
>> all classes on the classlist. The behavior is backwards compatible:
>>    - If no app/platform classes are loaded, dump time only reports 
>> error if non-empty directory exists in -Xbootclasspath/a path
>>    - If app/platform classes are loaded, dump time reports error for 
>> any non-empty directory exists in -Xbootclasspath/a path, class path, 
>> or module path
>>
>> - Removed unnecessary ‘if (!DumpSharedSpaces)’ from 
>> SharedClassUtil::initialize(). The code path is only executed when 
>> UseSharedSpaces is enabled.
>>
>> - Return immediately in 
>> SystemDictionaryShared::find_or_load_shared_class() if the archive 
>> header indicates there is no app or platform classes in the shared 
>> archive. This reduces overhead in class loading if the shared archive 
>> only contains system classes. It also provides backwards 
>> compatibility in cases where shared application classes should be 
>> disabled. For example, when the default system class loader is 
>> replaced by user provided loader, archived application classes are 
>> inactive (not loaded from archive) at runtime without affecting the 
>> use of archived system classes.
>>
>> - Changed GenerateLinkOptData.gmk to filter out HelloClasslist from 
>> the default classlist in JDK image, which is generated using 
>> DumpLoadedClassList option. Thanks for David and Ioi's input on this.
>>
>> - Updated various cds/appcds tests to reflect the JVM changes. The 
>> use of -XX:+UseAppCDS in tests remains unchanged.
>>
>>    - Removed -XX:+UnlockCommercialFeatures for -XX:+UseAppCDS in tests
>>
>>    - Removed -XX:+UnlockDiagnosticVMOptions for -XX:SharedArchiveFile 
>> in tests
>>
>>    - Updated NonBootLoaderClasses.java: ensure app/platform classes 
>> on classlist are archived without -XX:+UseAppCDS
>>
>>    - Updated DirClasspathTest.java: reflect the change for non-empty 
>> directory handling
>>
>>    - Updated MismatchedUseAppCDS.java:  -XX:-UseAppCDS has no effect
>>
>> Tested with all cds and appcds tests locally with both fastdebug and 
>> release builds. Tested with hs-teir1, hs-tier2, jdk-tier1 and 
>> jdk-tier2. The hs-tier3, hs-tier4 and hs-tier4 are in progress.
>>
>> Thanks,
>> Jiangli
>>