> On Apr 27, 2018, at 1:43 AM, David Holmes <david.hol...@oracle.com> wrote: > > On 27/04/2018 4:20 AM, Jiangli Zhou wrote: >>> 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”). > > I should have of course said "the word Dumping" and "in case we find 'Dumping > core'". :) > >> 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. > > No that's fine. Removing the file completely makes sense.
Thanks! Jiangli > > Thanks, > David > >> 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 >>>>>
