> On Apr 25, 2018, at 10:09 PM, David Holmes <david.hol...@oracle.com> wrote: > > On 26/04/2018 2:34 PM, Jiangli Zhou wrote: >> Here is the incremental webrev with updates that incorporate all feedbacks: >> http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev_inc.02/ > > Looks good.
Thanks! > > One additional comment on > test/hotspot/jtreg/runtime/appcds/SharedArchiveFile.java - it seems > insufficient to just check the output for the word "dump". I would expect to > see something more definitively associated with -Xshare:dump and also a check > that it exited cleanly (in case we find "core dump”). With other test cases being removed from appcds/SharedArchiveFile.java, I just realized that the test now essentially is the same as the jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java test. SharedArchiveFile/SharedArchiveFile.java includes more robust checks as you suggested and also tests runtime. Your comments above (and fresh morning coffee) reminded me that. :-) So I went ahead removed the appcds/SharedArchiveFile.java. Please let me know if you want to see the new webrev. Thanks! Jiangli > > Thanks, > David > ----- > >> - 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 >>>
