Hi David, > On Apr 25, 2018, at 3:12 PM, David Holmes <david.hol...@oracle.com> wrote: > > Hi Jiangli, > > On 26/04/2018 3:39 AM, Jiangli Zhou wrote: >> Hi David, >> Thanks a lot for the review! >>> On Apr 24, 2018, at 11:17 PM, David Holmes <david.hol...@oracle.com> 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? >> Done. Thanks for noticing that! >>> >>>> 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. >> The usages are left as backwards compatibility check. They also serve the >> testing purpose to make sure the presence of the flag does not cause any >> unexpected behavior. Those are the main reasons why I didn’t remove the flag >> usages in this webrev. > > This sounds like you are basically testing whether obsoleting the flag has > worked correctly.
Right, that was part of the intention. > You don't need to do that through formal testing. A simple run of "java > -XX:+UseAppCDS" should show the obsoletion warning and that's that. We don't > maintain an obsolete flag test the way we do for deprecated flags. That sounds reasonable. > > So IMHO there's really no reason to keep the flag in any of the tests. As I > said they will all have to be removed when the flag is expired in 12. Ok. I’ll remove the flag from the tests. Thanks! Jiangli > > Thanks, > David > >>> >>> 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. >> Will do. >>> >>> 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. >> Ok. >>> >>> 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. >> Sounds reasonable. I’ll remove UseAppCDS.java. There are existing tests that >> check app classes get archived and used. >> Thanks! >> Jiangli >>> >>> 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
