Hi Jiangli,
The code changes look good to me. I agree with David's comments about
the test cases.
Thanks
- Ioi
On 4/24/18 11:17 PM, 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/
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
