Hi Magnus, Thanks a lot for the review!
Jiangli > On Apr 25, 2018, at 2:17 AM, Magnus Ihse Bursie > <magnus.ihse.bur...@oracle.com> wrote: > > > > On 2018-04-25 08:17, 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/ > Build changes look good. > > /Magnus >> >> 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 >>> >
