Hi Erik, > On Apr 25, 2018, at 8:30 AM, Erik Joelsson <erik.joels...@oracle.com> wrote: > > Hello, > > I would prefer if the .tmp file was not deleted. That will make it easier to > debug problems in the future. We have recently had reason to look at the > contents of the files here to figure out if the generator ran properly. If > leaving it around, then perhaps call it .raw or something less temporary > sounding than .tmp?
Thanks for the suggestion. Sounds good to me, as long as it follows JDK build convention. Thanks a lot for looking at this! Jiangli > > /Erik > > > On 2018-04-25 02:17, Magnus Ihse Bursie wrote: >> >> >> On 2018-04-25 08:17, David Holmes 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? >>> >>>> webrev: http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev.00/ >> Build changes look good. >> >> /Magnus >>> >>> 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. >>> >>> 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. >>> >>> 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. >>> >>> 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. >>> >>> 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 >>>> >> >
