Here is the incremental webrev with updates that incorporate all feedbacks: http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev_inc.02/
- Filed JDK-8202282 (https://bugs.openjdk.java.net/browse/JDK-8202282) for TestCommon.makeCommandLineForAppCDS() cleanup. - Removed case 2, 3 and 4 in SharedArchiveFile.java. - Removed UseAppCDS.java test. - Removed UseAppCDS in various tests. - Changed to keep the original version of the classlist and renamed to classlist.raw. - Changed check_nonempty_dir_in_shared_path_table() to report all non-empty directories in the shared path table entries before exiting VM. Full webrev: http://java.se.oracle.com:10065/mdash/jobs/jianzhou-jdk-20180426-0406-20150 Tested all modified tests locally. Rerunning hs-tier1 ~ hs-tier5 tests. Thanks for all the suggestions! Jiangli > On Apr 25, 2018, at 5:24 PM, Jiangli Zhou <jiangli.z...@oracle.com> wrote: > > 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 >
