RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary (original) (raw)

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Wed Aug 29 11:53:38 UTC 2018

On 2018-08-28 18:25, Erik Joelsson wrote:

Build changes look good to me (but should probably get review from someone else).

I'm (as usually) not as happy as Erik. ;-)

In Images.gmk, you have added this rule:           @−Xshare:dump−Xmx128M−Xms128M@ -Xshare:dump -Xmx128M -Xms128M @−Xshare:dump−Xmx128M−Xms128M(LOG_INFO)

It took me a while to grasp. You are relying on $(JIMAGE_TARGET_FILE) to evaluate to bin/java. But that is only incidentally so. This file is just picked arbitrarily to represent when the entire image is done, for make. Please use $(JRE_IMAGE_DIR)/bin/java instead.

The logic in jdk-options.m4 is broken. If indeed it is not possible to use cds archive with zero, then things will break since it will still be built if using --enable-cds-archive!

What you should do is to set default to true unless using cross compilation or zero. If the user explicitly sets --enable-cds-archive, and it's not possible, a fatal error should be generated.

Apart from this, I'm more on Erik's line. :-)

/Magnus

/Erik

On 2018-08-27 13:33, Jiangli Zhou wrote: Please review the implementation for JEP JDK-8204247 (https://bugs.openjdk.java.net/browse/JDK-8204247). The goal of the JEP is to include a default CDS archive in JDK 12 binary distribution (downloadable from http://jdk.java.net/12/). The default CDS archive is generated using the default classlist (resides in the lib/ directory) at JDK build time. Any comments/suggestions are highly appreciated.

All makefile changes in the webrev are contributed by Erik Joelsson (many thanks!!). This is a combination of efforts from different teams and individuals. Thanks to everyone who has been involved in the JEP & implementation discussions, testing and bug fixing! JEP: https://bugs.openjdk.java.net/browse/JDK-8204247 RFE: https://bugs.openjdk.java.net/browse/JDK-8202951 webrev: http://cr.openjdk.java.net/~jiangli/8202951/webrev.00/ Two sanity test cases for the default CDS archive are included test/hotspot/jtreg/runtime/SharedArchiveFile. They are not intended for in-depth CDS functional testing, which is already covered by the existing cds/appcds tests and all tiered tests executing with the default CDS archive enabled. As part of the webrev, test/jdk/javax/imageio/plugins/png/ItxtUtf8Test.java is also fixed to use larger java heap (JDK-8209739 , https://bugs.openjdk.java.net/browse/JDK-8209739). Tests executed:  - several rounds of tier1 - tier8 via mach5  - JCK lang, api and vm tests via mach5

Thanks! Calvin, Ioi, Jiangli

More information about the jdk-dev mailing list