RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

Hi Calvin,

Thanks for the review!

> On Apr 25, 2018, at 10:13 AM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
> 
> Hi Jiangli,
> 
> I've only looked at the vm code changes and not the test changes. It looks good overall. I have the following minor comments:
> 
> I think the following function in classLoader.cpp can be removed since you're not using it anymore.
> 
> 639 #if INCLUDE_CDS
> 640 void ClassLoader::check_shared_classpath(const char *path) {
> 641   if (strcmp(path, "") == 0) {
> 642     exit_with_path_failure("Cannot have empty path in archived classpaths", NULL);
> 643   }
> 644
> 645   struct stat st;
> 646   if (os::stat(path, &st) == 0) {
> 647     if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
> 648       if (!os::dir_is_empty(path)) {
> 649         tty->print_cr("Error: non-empty directory '%s'", path);
> 650         exit_with_path_failure("CDS allows only empty directories in archived classpaths", NULL);
> 651       }
> 652     }
> 653   }
> 654 }
> 655 #endif

Good catch! I remember removing this function specifically. It probably resurrected after several merges and editing. Will remove.

> 
> I see that non-empty directory check is being done in metaspaceShared.cpp after processing all the classpath and module path entries. I think in FileMapInfo::check_nonempty_dir_in_shared_path_table(), instead of exiting upon encountering the first non-empty dir, we can iterate all entries and print out all the entries with non-empty dir before exiting. In each of the "Error: non-empty directory" message, we can consider printing out the type of entry (classpath or module path) if that info is readily available. This would give more info to the user at one time.

That’s a good suggestion! My original goal was to maintain the old behavior. I like your idea of providing more information at once and we now have the ability to do that. Will do.

Thanks!

Jiangli

> 
> thanks,
> Calvin
> 
> On 4/24/18, 5:53 PM, 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
>> 	webrev: http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev.00/
>> 
>> 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
>>