> On Apr 26, 2018, at 12:21 PM, Calvin Cheung <calvin.che...@oracle.com> wrote: > > > > On 4/26/18, 12:00 PM, Jiangli Zhou wrote: >> Hi Calvin, >> >>> On Apr 26, 2018, at 10:10 AM, Calvin Cheung<calvin.che...@oracle.com> >>> wrote: >>> >>> >>> >>> On 4/25/18, 9: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! >> >>>> - Filed JDK-8202282 (https://bugs.openjdk.java.net/browse/JDK-8202282) for >>>> TestCommon.makeCommandLineForAppCDS() cleanup. >>>> - Removed case 2, 3 and 4 in SharedArchiveFile.java. >>> To address David's comment on checking the result of -Xshare:dump, you can >>> replace >>> 52 out.shouldContain("Dumping"); >>> >>> with >>> TestCommon.checkDump(out); >>> >>> checkDump() contains the following check: >>> output.shouldContain("Loading classes to share"); >>> >>> No need to generate another webrev if you make the above change. >> I removed appcds/SharedArchiveFile.java (please see my reply to David for >> details). >> >>>> - Removed UseAppCDS.java test. >>> Since you've removed the UseAppCDS.java, I think the UseAppCDS_Test.java >>> should also be removed. >>> It would involve changing the GraalWithLimitedMetaspace.java; the test >>> could just use the test-classes/Hello.java instead of UseAppCDS_Test.java. >>> This cleanup can be done later, perhaps as part of the bug you've filed. >> Could you please specify the connection between UseAppCDS.java and >> UseAppCDS_Test.java? > UseAppCDS.java dump the classlist by running the UseAppCDS_Test. > It then creates an archive based on the classlist. > It then runs the UseAppCDS_Test with the archive. > > The UseAppCDS_Test simply does the following: > public class UseAppCDS_Test { > // args are from UseAppCDS: > // args[0] = TEST_OUT > public static void main(String[] args) { > System.out.println(args[0]); > } > } > > GraalWithLimitedMetaspace.java depends on UseAppCDS_Test in a similar way but > it only performs dumping.
Ok, since GraalWithLimitedMetaspace.java still uses UseAppCDS_Test.java, it is better to keep it for now. Thanks, Jiangli > > thanks, > Calvin >> >>>> - 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 >>> I think the correct URL is: >>> http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev.02/ ? >> Yes. >> >> Thanks! >> >> Jiangli >> >>> thanks, >>> Calvin >>>> 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
