Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
Dear all, JEP 341, Default CDS Archive has been targeted to JDK 12 (please see Mark's email from yesterday, Re: JEP proposed to target JDK 12: 341: Default CDS Archives). Code review, testing and performance runs are completed for the default CDS archive changes. I've started the final pre-integration test yesterday with all tier1 - tier8. So far most of the tiers have been completed. Just a heads up, I'm planning to integrate the change in the next few hour. Thanks everyone for contributing to this effort!!! Jiangli On 9/6/18 12:45 PM, Jiangli Zhou wrote: Hi, The JEP (http://openjdk.java.net/jeps/341) for the default CDS archives is now a candidate. Please see Mark's email, 'New candidate JEP: 341: Default CDS Archives'. Please help test Erik's makefile patch for your platform if it is not built/tested in openjdk mainline to prevent any possible breakage on your side. Any comments/feedbacks on the default CDS archive are highly appreciated! http://cr.openjdk.java.net/~jiangli/8202951/webrev.02/ The above webrev is sync'ed with the lasted jdk/jdk repository today. Thanks! Jiangli On 8/30/18 11:26 AM, Jiangli Zhou wrote: Hi Magnus, Sounds good. Will update the message. Thanks! Jiangli On 8/30/18 3:56 AM, Magnus Ihse Bursie wrote: On 2018-08-29 17:45, Erik Joelsson wrote: Hello Magnus, As the author of the build changes I will answer this. On 2018-08-29 04:53, Magnus Ihse Bursie wrote: 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 $(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. I can agree with this part. That was a bit of a hack done in a hurry. 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. Here I'm not as sure. I deliberately let it be possible to override the default behavior for zero as someone might want to fix that at some point, you never know. For cross compilation it's just not possible ever so that's different. Maybe my reasoning is invalid. I see. I still think it would have made more sense, in that case, to set the default to false if using zero, but allow override. But I'm not going to insist on that. However, if the problem with zero is not that it's technically impossible, just that it's not tested, I think the message should be changed from "[no, not possible with JVM variant zero]" to just "[no]" with a comment in the configure script that it's off by default since it's not tested. Apart from that, Jiangli's latest webrev looks good. Jiangli: If you fix it like I suggested, you do not need to respin the webrev. /Magnus /Erik 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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
Hi, The JEP (http://openjdk.java.net/jeps/341) for the default CDS archives is now a candidate. Please see Mark's email, 'New candidate JEP: 341: Default CDS Archives'. Please help test Erik's makefile patch for your platform if it is not built/tested in openjdk mainline to prevent any possible breakage on your side. Any comments/feedbacks on the default CDS archive are highly appreciated! http://cr.openjdk.java.net/~jiangli/8202951/webrev.02/ The above webrev is sync'ed with the lasted jdk/jdk repository today. Thanks! Jiangli On 8/30/18 11:26 AM, Jiangli Zhou wrote: Hi Magnus, Sounds good. Will update the message. Thanks! Jiangli On 8/30/18 3:56 AM, Magnus Ihse Bursie wrote: On 2018-08-29 17:45, Erik Joelsson wrote: Hello Magnus, As the author of the build changes I will answer this. On 2018-08-29 04:53, Magnus Ihse Bursie wrote: 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 $(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. I can agree with this part. That was a bit of a hack done in a hurry. 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. Here I'm not as sure. I deliberately let it be possible to override the default behavior for zero as someone might want to fix that at some point, you never know. For cross compilation it's just not possible ever so that's different. Maybe my reasoning is invalid. I see. I still think it would have made more sense, in that case, to set the default to false if using zero, but allow override. But I'm not going to insist on that. However, if the problem with zero is not that it's technically impossible, just that it's not tested, I think the message should be changed from "[no, not possible with JVM variant zero]" to just "[no]" with a comment in the configure script that it's off by default since it's not tested. Apart from that, Jiangli's latest webrev looks good. Jiangli: If you fix it like I suggested, you do not need to respin the webrev. /Magnus /Erik 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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
Hi Magnus, Sounds good. Will update the message. Thanks! Jiangli On 8/30/18 3:56 AM, Magnus Ihse Bursie wrote: On 2018-08-29 17:45, Erik Joelsson wrote: Hello Magnus, As the author of the build changes I will answer this. On 2018-08-29 04:53, Magnus Ihse Bursie wrote: 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 $(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. I can agree with this part. That was a bit of a hack done in a hurry. 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. Here I'm not as sure. I deliberately let it be possible to override the default behavior for zero as someone might want to fix that at some point, you never know. For cross compilation it's just not possible ever so that's different. Maybe my reasoning is invalid. I see. I still think it would have made more sense, in that case, to set the default to false if using zero, but allow override. But I'm not going to insist on that. However, if the problem with zero is not that it's technically impossible, just that it's not tested, I think the message should be changed from "[no, not possible with JVM variant zero]" to just "[no]" with a comment in the configure script that it's off by default since it's not tested. Apart from that, Jiangli's latest webrev looks good. Jiangli: If you fix it like I suggested, you do not need to respin the webrev. /Magnus /Erik 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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
On 2018-08-29 17:45, Erik Joelsson wrote: Hello Magnus, As the author of the build changes I will answer this. On 2018-08-29 04:53, Magnus Ihse Bursie wrote: 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 $(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. I can agree with this part. That was a bit of a hack done in a hurry. 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. Here I'm not as sure. I deliberately let it be possible to override the default behavior for zero as someone might want to fix that at some point, you never know. For cross compilation it's just not possible ever so that's different. Maybe my reasoning is invalid. I see. I still think it would have made more sense, in that case, to set the default to false if using zero, but allow override. But I'm not going to insist on that. However, if the problem with zero is not that it's technically impossible, just that it's not tested, I think the message should be changed from "[no, not possible with JVM variant zero]" to just "[no]" with a comment in the configure script that it's off by default since it's not tested. Apart from that, Jiangli's latest webrev looks good. Jiangli: If you fix it like I suggested, you do not need to respin the webrev. /Magnus /Erik 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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
Hi Erik and Magnus, I updated Images.gmk accordingly. Magnus, thanks for reviewing! http://cr.openjdk.java.net/~jiangli/8202951/webrev.02/make/Images.gmk.frames.html Complete webrev: http://cr.openjdk.java.net/~jiangli/8202951/webrev.02/ Please see comments below. On 8/29/18 8:45 AM, Erik Joelsson wrote: Hello Magnus, As the author of the build changes I will answer this. On 2018-08-29 04:53, Magnus Ihse Bursie wrote: 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 $(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. I can agree with this part. That was a bit of a hack done in a hurry. 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. Here I'm not as sure. I deliberately let it be possible to override the default behavior for zero as someone might want to fix that at some point, you never know. For cross compilation it's just not possible ever so that's different. Maybe my reasoning is invalid. I think that's reasonable. Thanks! Jiangli /Erik 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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
Hello Magnus, As the author of the build changes I will answer this. On 2018-08-29 04:53, Magnus Ihse Bursie wrote: 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 $(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. I can agree with this part. That was a bit of a hack done in a hurry. 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. Here I'm not as sure. I deliberately let it be possible to override the default behavior for zero as someone might want to fix that at some point, you never know. For cross compilation it's just not possible ever so that's different. Maybe my reasoning is invalid. /Erik 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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
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 $(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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
Looks good. Thanks!
- Ioi
On 8/28/18 6:32 PM, Jiangli Zhou wrote:
Here is the updated webre with CheckDefaultArchiveFile.java changes.
http://cr.openjdk.java.net/~jiangli/8202951/webrev.01/
Thanks,
Jiangli
On 8/28/18 11:09 AM, Jiangli Zhou wrote:
On 8/28/18 9:33 AM, Ioi Lam wrote:
The JVM and test changes look good. I just have one comment:
CheckDefaultArchiveFile.java
51 if (!Platform.isDefaultCDSArchiveSupported()) {
52 if (Files.notExists(jsa)) {
53 System.out.println("Passed. " + vmString +
54 ": no default classes.jsa
file");
55 } else {
56 throw new RuntimeException(vmString + "contains
" + jsaString);
57 }
People may manually do "java -Xshare:dump" on their own platforms
and them run the hotspot tests. It seems too strict to treat this as
an error.
I think this block should be removed.
That's probably a common scenario. I agree, it is too strict. Will
remove the block.
Thanks!
Jiangli
Thanks
- Ioi
On 8/28/18 9:25 AM, Erik Joelsson wrote:
Build changes look good to me (but should probably get review from
someone else).
/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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
Here is the updated webre with CheckDefaultArchiveFile.java changes.
http://cr.openjdk.java.net/~jiangli/8202951/webrev.01/
Thanks,
Jiangli
On 8/28/18 11:09 AM, Jiangli Zhou wrote:
On 8/28/18 9:33 AM, Ioi Lam wrote:
The JVM and test changes look good. I just have one comment:
CheckDefaultArchiveFile.java
51 if (!Platform.isDefaultCDSArchiveSupported()) {
52 if (Files.notExists(jsa)) {
53 System.out.println("Passed. " + vmString +
54 ": no default classes.jsa
file");
55 } else {
56 throw new RuntimeException(vmString + "contains
" + jsaString);
57 }
People may manually do "java -Xshare:dump" on their own platforms and
them run the hotspot tests. It seems too strict to treat this as an
error.
I think this block should be removed.
That's probably a common scenario. I agree, it is too strict. Will
remove the block.
Thanks!
Jiangli
Thanks
- Ioi
On 8/28/18 9:25 AM, Erik Joelsson wrote:
Build changes look good to me (but should probably get review from
someone else).
/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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
On 8/28/18 9:33 AM, Ioi Lam wrote:
The JVM and test changes look good. I just have one comment:
CheckDefaultArchiveFile.java
51 if (!Platform.isDefaultCDSArchiveSupported()) {
52 if (Files.notExists(jsa)) {
53 System.out.println("Passed. " + vmString +
54 ": no default classes.jsa file");
55 } else {
56 throw new RuntimeException(vmString + "contains "
+ jsaString);
57 }
People may manually do "java -Xshare:dump" on their own platforms and
them run the hotspot tests. It seems too strict to treat this as an
error.
I think this block should be removed.
That's probably a common scenario. I agree, it is too strict. Will
remove the block.
Thanks!
Jiangli
Thanks
- Ioi
On 8/28/18 9:25 AM, Erik Joelsson wrote:
Build changes look good to me (but should probably get review from
someone else).
/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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
Thanks, Erik! Jiangli On 8/28/18 9:25 AM, Erik Joelsson wrote: Build changes look good to me (but should probably get review from someone else). /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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
The JVM and test changes look good. I just have one comment:
CheckDefaultArchiveFile.java
51 if (!Platform.isDefaultCDSArchiveSupported()) {
52 if (Files.notExists(jsa)) {
53 System.out.println("Passed. " + vmString +
54 ": no default classes.jsa file");
55 } else {
56 throw new RuntimeException(vmString + "contains "
+ jsaString);
57 }
People may manually do "java -Xshare:dump" on their own platforms and
them run the hotspot tests. It seems too strict to treat this as an error.
I think this block should be removed.
Thanks
- Ioi
On 8/28/18 9:25 AM, Erik Joelsson wrote:
Build changes look good to me (but should probably get review from
someone else).
/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
Re: RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
Build changes look good to me (but should probably get review from someone else). /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
RFR: JDK-8202951: Implementation of JEPJDK-8204247: Include default CDS (Class Data Sharing) archive in JDK binary
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
