Re: RFR(XS): 8211837: Creation of the default CDS Archive should depend on ENABLE_CDS

[Jiangli Zhou]

Hi Volker,

Looks good. Thanks for fixing! It would be good to update the following 
comment in jdk-options.m4 to add AIX, minimal and core.


 609 


 610 #
 611 # Disable the default CDS archive generation
 612 #   cross compilation - disabled
 613 #   zero  - off by default (not a tested configuration)
 614 #


Thanks!

Jiangli


On 10/8/18 3:10 AM, Volker Simonis wrote:

Hi Martin, Goetz,

thanks for reviewing my patch! As Aleksey posted a similar patch for
fixing the Zero build I've extended my patch to handle
zero/minimal/core as well.

In fact the patch now disables CDS on AIX, minimal, core and zero
unless the user explicitly requests it with the help of
`--with-jvm-features=cds`. The configuration variant with
`--with-jvm-features=-cds` should now also be handled correctly (I
hope caught all possible combinations :)

If CDS is disabled, the creation of the default class list and default
archive will now be disabled as well.

@Aleksey Shipilev : can you please check if this new version of my
patch also works for you?

http://cr.openjdk.java.net/~simonis/webrevs/2018/8211837.v1/

Thank you and best regards,
Volker
On Mon, Oct 8, 2018 at 11:10 AM Doerr, Martin  wrote:

Hi Volker,

looks good. Thanks for fixing.
Of course, it would be great if this could be used to fix minimal/zero build, 
too.

Best regards,
Martin


-Original Message-
From: hotspot-runtime-dev  On 
Behalf Of Volker Simonis
Sent: Montag, 8. Oktober 2018 10:19
To: build-dev ; hotspot-runtime-...@openjdk.java.net 
runtime 
Subject: RFR(XS): 8211837: Creation of the default CDS Archive should depend on 
ENABLE_CDS

Hi,

can I please have a review for the following trivial build fix:

http://cr.openjdk.java.net/~simonis/webrevs/2018/8211837/
https://bugs.openjdk.java.net/browse/JDK-8211837

It makes no sense to try to create a default CDS archive on VMs which
don't support CDS at all. We already have '--enable_cds' which
defaults to 'true' on all platforms except AIX.

The check for '--enable_cds_archive' should use the result of
'--enable_cds' (which is saved in "ENABLE_CDS") and only enable
default archive creation if CDS is enabled.

Failure to do so currently breaks the AIX build (after JDK-)8202951 was pushed).

Thank you and best regards,
Volker

Re: RFR: JDK-8210592: Convert CDS-mode test sets in tier5 and tier6 to non-CDS-mode testing

[Jiangli Zhou]

Thanks, David!

Jiangli


On 9/17/18 4:45 PM, David Holmes wrote:

Hi Jiangli,

Conversion looks good.

Thanks,
David

On 18/09/2018 8:15 AM, Jiangli Zhou wrote:
Please review the change for JEP 341 (Default CDS Archives) sub-task, 
JDK-8210592.


Currently, there are sub-sets of tiered tests running in CDS mode 
(defined in closed tier5 and tier6 test definitions). These tests are 
also executed in 'normal' mode in various tiers. GENERATE_CDS_ARCHIVE 
is used to create a CDS archive using the default classlist before 
executing those tests in CDS-mode.


When the default CDS archive is enabled, it is no longer necessary to 
execute those tests in CDS mode explicitly since all tiered testing 
enables the default CDS archive by default. The change in the webrev 
removes GENERATE_CDS_ARCHIVE. To increase test coverage, the test 
sets in CDS mode are converted to run in non-CDS mode (with 
-Xshare:off enabled explicitly) in tier5 and tier6. The conversion is 
done in the closed repo.


   webrev: http://cr.openjdk.java.net/~jiangli/8210592/webrev.00/

   JEP sub-task: https://bugs.openjdk.java.net/browse/JDK-8210592

Tested tier5 and tier6 with the default CDS archive patch enabled.

Thanks,

Jiangli

Re: RFR: JDK-8210592: Convert CDS-mode test sets in tier5 and tier6 to non-CDS-mode testing

[Jiangli Zhou]

Thanks, Erik!

Jiangli


On 9/17/18 4:42 PM, Erik Joelsson wrote:

Looks good.

/Erik


On 2018-09-17 15:15, Jiangli Zhou wrote:
Please review the change for JEP 341 (Default CDS Archives) sub-task, 
JDK-8210592.


Currently, there are sub-sets of tiered tests running in CDS mode 
(defined in closed tier5 and tier6 test definitions). These tests are 
also executed in 'normal' mode in various tiers. GENERATE_CDS_ARCHIVE 
is used to create a CDS archive using the default classlist before 
executing those tests in CDS-mode.


When the default CDS archive is enabled, it is no longer necessary to 
execute those tests in CDS mode explicitly since all tiered testing 
enables the default CDS archive by default. The change in the webrev 
removes GENERATE_CDS_ARCHIVE. To increase test coverage, the test 
sets in CDS mode are converted to run in non-CDS mode (with 
-Xshare:off enabled explicitly) in tier5 and tier6. The conversion is 
done in the closed repo.


  webrev: http://cr.openjdk.java.net/~jiangli/8210592/webrev.00/

  JEP sub-task: https://bugs.openjdk.java.net/browse/JDK-8210592

Tested tier5 and tier6 with the default CDS archive patch enabled.

Thanks,

Jiangli

Re: RFR: JDK-8210592: Convert CDS-mode test sets in tier5 and tier6 to non-CDS-mode testing

[Jiangli Zhou]

Thanks, Misha!

Jiangli 

> On Sep 17, 2018, at 4:17 PM, mikhailo  wrote:
> 
> Change looks good,
> 
> Misha
> 
> 
>> On 09/17/2018 03:15 PM, Jiangli Zhou wrote:
>> Please review the change for JEP 341 (Default CDS Archives) sub-task, 
>> JDK-8210592.
>> 
>> Currently, there are sub-sets of tiered tests running in CDS mode (defined 
>> in closed tier5 and tier6 test definitions). These tests are also executed 
>> in 'normal' mode in various tiers. GENERATE_CDS_ARCHIVE is used to create a 
>> CDS archive using the default classlist before executing those tests in 
>> CDS-mode.
>> 
>> When the default CDS archive is enabled, it is no longer necessary to 
>> execute those tests in CDS mode explicitly since all tiered testing enables 
>> the default CDS archive by default. The change in the webrev removes 
>> GENERATE_CDS_ARCHIVE. To increase test coverage, the test sets in CDS mode 
>> are converted to run in non-CDS mode (with -Xshare:off enabled explicitly) 
>> in tier5 and tier6. The conversion is done in the closed repo.
>> 
>>   webrev: http://cr.openjdk.java.net/~jiangli/8210592/webrev.00/
>> 
>>   JEP sub-task: https://bugs.openjdk.java.net/browse/JDK-8210592
>> 
>> Tested tier5 and tier6 with the default CDS archive patch enabled.
>> 
>> Thanks,
>> 
>> Jiangli
>> 
> 

RFR: JDK-8210592: Convert CDS-mode test sets in tier5 and tier6 to non-CDS-mode testing

[Jiangli Zhou]

Please review the change for JEP 341 (Default CDS Archives) sub-task, 
JDK-8210592.


Currently, there are sub-sets of tiered tests running in CDS mode 
(defined in closed tier5 and tier6 test definitions). These tests are 
also executed in 'normal' mode in various tiers. GENERATE_CDS_ARCHIVE is 
used to create a CDS archive using the default classlist before 
executing those tests in CDS-mode.


When the default CDS archive is enabled, it is no longer necessary to 
execute those tests in CDS mode explicitly since all tiered testing 
enables the default CDS archive by default. The change in the webrev 
removes GENERATE_CDS_ARCHIVE. To increase test coverage, the test sets 
in CDS mode are converted to run in non-CDS mode (with -Xshare:off 
enabled explicitly) in tier5 and tier6. The conversion is done in the 
closed repo.


  webrev: http://cr.openjdk.java.net/~jiangli/8210592/webrev.00/

  JEP sub-task: https://bugs.openjdk.java.net/browse/JDK-8210592

Tested tier5 and tier6 with the default CDS archive patch enabled.

Thanks,

Jiangli

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

[Jiangli Zhou]

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

[Jiangli Zhou]

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

[Jiangli Zhou]

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

[Jiangli Zhou]

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

[Jiangli Zhou]

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

[Jiangli Zhou]

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

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

[Jiangli Zhou]

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(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules

[Jiangli Zhou]

Thanks a lot for reviewing, Mandy!

Jiangli

> On Jul 6, 2018, at 1:40 PM, mandy chung  wrote:
> 
> Hi Jiangli,
> 
> On 6/28/18 4:15 PM, Jiangli Zhou wrote:> webrev: 
> http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921
> 
> Good work.  I'm glad to see a pretty good startup improvement.
> 
> I reviewed java.base change that looks good.
> 
> Mandy

Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules

[Jiangli Zhou]

Hi Ioi,

Thanks for the review!

> On Jul 5, 2018, at 5:45 PM, Ioi Lam  wrote:
> 
> Hi Jiangli,
> 
> Thank you so much for working on this. I think it's great that we can get the
> start-up improvement by archiving the ModuleDescriptor.
> 
> I just have some coding style comments regarding heapShared.cpp. This file
> contains the code for coping objects and relocating pointers. By its nature,
> this kind of code is usually complicated, so I think we should try to make
> it as easy to understand as possible.
> 
> 
> [1] HeapShared::walk_from_field_and_archiving:
> 
> This name is not grammatically correct. How about
> HeapShared::archive_reachable_objects_from_static_field

Sounds good.

> 
> [2] How about changing the parameter field_offset -> static_field_offset
> When I first read the code I was confused whether it's talking
> about static or instance fields. Usually, "field"
> implies instance field, so it's better to specifically
> say "static field”.

Ok.

> 
> [3] This code would fail if "f" is already archived.
> 
> 473   // get the archived copy of the field referenced object
> 474   oop af = MetaspaceShared::archive_heap_object(f, THREAD);
> 475   WalkOopAndArchiveClosure walker(1, subgraph_info, f, af);
> 476   f->oop_iterate();

Hmmm, it’s possible we might encounter an archived object during reference 
walking & archiving in future cases. I’ll add a check.

> 
> [4] There's duplicated code between walk_from_field_and_archiving and
> WalkOopAndArchiveClosure::do_oop_work
> 
> 403   assert(relocated_k == MetaspaceShared::get_relocated_klass(orig_k),
> 404  "must be the relocated Klass in the shared space");
> 405   _subgraph_info->add_subgraph_object_klass(orig_k, relocated_k);
> 
> - vs -
> 
> 484   assert(relocated_k == MetaspaceShared::get_relocated_klass(orig_k),
> 485  "must be the relocated Klass in the shared space");
> 486   subgraph_info->add_subgraph_object_klass(orig_k, relocated_k);

I’ll move the assert into add_subgraph_object_klass().

> 
> [5] This code  is also duplicated:
> 
> 375   RawAccess::oop_store(new_p, archived);
> 376   log.print("--- archived copy existing, store archived " PTR_FORMAT 
> " in " PTR_FORMAT,
> 377 p2i(archived), p2i(new_p));
> 
> - vs -
> 
> 395  RawAccess::oop_store(new_p, archived);
> 396  log.print("=== store archived " PTR_FORMAT " in " PTR_FORMAT,
> 397p2i(archived), p2i(new_p));

The first case is for existing archived copy and the second is for newly 
archived. The different logging messages are helpful for debugging. Not sure if 
using a function to encapsulate the store & log worth it in this case. Any 
suggestion?

> 
> [6] This code, even though it's correct, is hard to understand --
> why are we calculating the distance between the two objects?

> 
> 368  size_t delta = pointer_delta((HeapWord*)_archived_referencing_obj,
> 369 (HeapWord*)_orig_referencing_obj);
> 370  T* new_p = (T*)((HeapWord*)p + delta);
> 
> I thin it would be easier to understand if we change the order of the
> two arithmetic operations:
> 
> // new_p is the address of the same field inside 
> _archived_referencing_obj.
> size_t field_offset_in_bytes = pointer_delta(p, _orig_referencing_obj, 1);
> T* new_p = (T*)(address(_orig_referencing_obj) + field_offset_in_bytes);

I think this works too. I’ll change as you suggested.

> 
> [7] I have a hard time understand this log:
> 
> 376   log.print("--- archived copy existing, store archived " PTR_FORMAT 
> " in " PTR_FORMAT,
> 377 p2i(archived), p2i(new_p));
> 
> How about this?
> 
> log.print("--- updated embedded pointer @[" PTR_FORMAT "] => " PTR_FORMAT,
>   p2i(new_p), p2i(archived));

It is for the case where there is an existing copy of the archived object. 
Maybe ‘found existing archived copy’ would help?

> 
> 
> For your consideration, I've incorporated my comments above into 
> heapShared.cpp.
> I've not tested it so it most likely won't build :-(
> 
> 
> http://cr.openjdk.java.net/~iklam/misc/heapShared.old.cpp  [your version]
> http://cr.openjdk.java.net/~iklam/misc/heapShared.new.cpp  [my version]
> 
> Please take a look and see if you like it.

Thanks a lot! I’ll take a look and incorporate your suggestions.

Thanks again!
Jiangli

> 
> Thanks
> - Ioi
> 
> On 6/28/18 4:15 PM, Jiangli Zhou wrote:
>> This is a follow-up RFE of JDK-8201650 (Move iteration order ran

Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules

[Jiangli Zhou]

Hi Erik,

Thank you for the quick review!

Jiangli

> On Jun 28, 2018, at 5:44 PM, Erik Joelsson  wrote:
> 
> Build changes look good.
> 
> /Erik
> 
> 
> On 2018-06-28 16:15, Jiangli Zhou wrote:
>> This is a follow-up RFE of JDK-8201650 (Move iteration order randomization 
>> of unmodifiable Set and Map to iterators), which was resolved to allow 
>> Set/Map objects being archived at CDS dump time (thanks Claes and Stuart 
>> Marks). In the current RFE, it archives the set of system ModuleReference 
>> and ModuleDescriptor objects (including their referenced objects) in 'open' 
>> archive heap region at CDS dump time. It allows reusing of the objects and 
>> bypassing the process of creating the system ModuleDescriptors and 
>> ModuleReferences at runtime for startup improvement. My preliminary 
>> measurements on linux-x64 showed ~5% startup improvement when running 
>> HelloWorld from -cp using archived module objects at runtime (without extra 
>> tuning).
>> 
>> The library changes in the following webrev are contributed by Alan Bateman. 
>> Thanks Alan and Mandy for discussions and help. Thanks Karen, Lois and Ioi 
>> for discussion and suggestions on initialization ordering.
>> 
>> The majority of the module object archiving code are in heapShared.hpp and 
>> heapShared.cpp. Thanks Coleen for pre-review and Eric Caspole for helping 
>> performance tests.
>> 
>> webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921
>> 
>> Tested using tier1 - tier6 via mach5 including all new test cases added in 
>> the webrev.
>> 
>> Following are the details of system module archiving, which are duplicated 
>> in above bug report.
>> ---
>> Support archiving system module graph when the initial module is unnamed 
>> module from -cp currently.
>> 
>> Support G1 GC, 64-bit (non-Windows). Requires UseCompressedOops and 
>> UseCompressedClassPointers.
>> 
>> Dump time system module object archiving
>> =
>> At dump time, the following fields in ArchivedModuleGraph are set to record 
>> the system module information created by ModuleBootstrap for archiving.
>> 
>>  private static SystemModules archivedSystemModules;
>>  private static ModuleFinder archivedSystemModuleFinder;
>>  private static String archivedMainModule;
>> 
>> The archiving process starts from a given static field in 
>> ArchivedModuleGraph class instance (java mirror object). The process 
>> archives the complete network of java heap objects that are reachable 
>> directly or indirectly from the starting object by following references.
>> 
>> 1. Starts from a given static field within the Class instance (java mirror). 
>> If the static field is a refererence field and points to a non-null java 
>> object, proceed to the next step. The static field and it's value is 
>> recorded and stored outside the archived mirror.
>> 2. Archives the referenced java object. If an archived copy of the current 
>> object already exists, updates the pointer in the archived copy of the 
>> referencing object to point to the current archived object. Otherwise, 
>> proceed to the next step.
>> 3. Follows all references within the current java object and recursively 
>> archive the sub-graph of objects starting from each reference encountered 
>> within the object.
>> 4. Updates the pointer in the archived copy of referecing object to point to 
>> the current archived object.
>> 5. The Klass of the current java object is added to a list of Klasses for 
>> loading and initializing before any object in the archived graph can be 
>> accessed at runtime.
>> 
>> Runtime initialization from archived system module objects
>> 
>> VM.initializeFromArchive() is called from ArchivedModuleGraph's 
>> static initializer to initialize from the archived module information. 
>> Klasses in the recorded list are loaded, linked and initialized. The static 
>> fields in ArchivedModuleGraph class instance are initialized using the 
>> archived field values. After initialization, the archived system module 
>> objects can be used directly.
>> 
>> If the archived java heap data is not successfully mapped at runtime, or 
>> there is an error during VM.initializeFromArchive(), then all static fields 
>> in ArchivedModuleGraph are not initialized. In that case, system 
>> ModuleDescriptor and ModuleReference objects are created as normal.
>> 
>> In non-CDS mode, VM.initializeFromArchive() returns immediately with 
>> minimum added overhead for normal execution.
>> 
>> Thanks,
>> Jiangli
>> 
>> 
> 

RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules

[Jiangli Zhou]

This is a follow-up RFE of JDK-8201650 (Move iteration order randomization of 
unmodifiable Set and Map to iterators), which was resolved to allow Set/Map 
objects being archived at CDS dump time (thanks Claes and Stuart Marks). In the 
current RFE, it archives the set of system ModuleReference and ModuleDescriptor 
objects (including their referenced objects) in 'open' archive heap region at 
CDS dump time. It allows reusing of the objects and bypassing the process of 
creating the system ModuleDescriptors and ModuleReferences at runtime for 
startup improvement. My preliminary measurements on linux-x64 showed ~5% 
startup improvement when running HelloWorld from -cp using archived module 
objects at runtime (without extra tuning).

The library changes in the following webrev are contributed by Alan Bateman. 
Thanks Alan and Mandy for discussions and help. Thanks Karen, Lois and Ioi for 
discussion and suggestions on initialization ordering.

The majority of the module object archiving code are in heapShared.hpp and 
heapShared.cpp. Thanks Coleen for pre-review and Eric Caspole for helping 
performance tests.

webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/
RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921

Tested using tier1 - tier6 via mach5 including all new test cases added in the 
webrev.

Following are the details of system module archiving, which are duplicated in 
above bug report.
---
Support archiving system module graph when the initial module is unnamed module 
from -cp currently. 

Support G1 GC, 64-bit (non-Windows). Requires UseCompressedOops and 
UseCompressedClassPointers. 

Dump time system module object archiving 
= 
At dump time, the following fields in ArchivedModuleGraph are set to record the 
system module information created by ModuleBootstrap for archiving. 

 private static SystemModules archivedSystemModules;  
 private static ModuleFinder archivedSystemModuleFinder;  
 private static String archivedMainModule; 

The archiving process starts from a given static field in ArchivedModuleGraph 
class instance (java mirror object). The process archives the complete network 
of java heap objects that are reachable directly or indirectly from the 
starting object by following references. 

1. Starts from a given static field within the Class instance (java mirror). If 
the static field is a refererence field and points to a non-null java object, 
proceed to the next step. The static field and it's value is recorded and 
stored outside the archived mirror. 
2. Archives the referenced java object. If an archived copy of the current 
object already exists, updates the pointer in the archived copy of the 
referencing object to point to the current archived object. Otherwise, proceed 
to the next step. 
3. Follows all references within the current java object and recursively 
archive the sub-graph of objects starting from each reference encountered 
within the object. 
4. Updates the pointer in the archived copy of referecing object to point to 
the current archived object. 
5. The Klass of the current java object is added to a list of Klasses for 
loading and initializing before any object in the archived graph can be 
accessed at runtime. 

Runtime initialization from archived system module objects 
 
VM.initializeFromArchive() is called from ArchivedModuleGraph's static 
initializer to initialize from the archived module information. Klasses in the 
recorded list are loaded, linked and initialized. The static fields in 
ArchivedModuleGraph class instance are initialized using the archived field 
values. After initialization, the archived system module objects can be used 
directly. 

If the archived java heap data is not successfully mapped at runtime, or there 
is an error during VM.initializeFromArchive(), then all static fields in 
ArchivedModuleGraph are not initialized. In that case, system ModuleDescriptor 
and ModuleReference objects are created as normal.

In non-CDS mode, VM.initializeFromArchive() returns immediately with 
minimum added overhead for normal execution.

Thanks,
Jiangli

Re: RFR(M): 8204965: Fix '--disable-cds' and disable CDS on AIX by default

[Jiangli Zhou]

> On Jun 19, 2018, at 5:21 AM, Volker Simonis  wrote:
> 
> On Tue, Jun 19, 2018 at 9:25 AM, David Holmes  wrote:
>> Hi Volker,
>> 
>> On 19/06/2018 4:50 PM, Volker Simonis wrote:
>>> 
>>> On Tue, Jun 19, 2018 at 6:54 AM, David Holmes 
>>> wrote:
 
 Hi Volker,
 
 v3 looks much cleaner - thanks.
 
 But AFAICS the change to jvmtiEnv.cpp is also not needed.
 ClassLoaderExt::append_boot_classpath exists regardless of INCLUDE_CDS
 but
 operates differently (just calling
 ClassLoader::add_to_boot_append_entries).
 
>>> 
>>> That's not entirely true because the whole compilation unit (i.e.
>>> classLoaderExt.cpp) which contains
>>> 'ClassLoaderExt::append_boot_classpath()' is excluded from the
>>> compilation if CDS is disabled (see make/hotspot/lib/JvmFeatures.gmk).
>> 
>> 
>> Hmmm. There's a CDS bug there. Either classLoaderExt.cpp should not be
>> excluded from a non-CDS build, or it should not contain any INCLUDE_CDS
>> guards! I suspect it should not be excluded.
>> 
>>> So I can either move the whole implementation of
>>> 'ClassLoaderExt::append_boot_classpath()' into classLoaderExt.hpp in
>>> which case things would work as you explained and my changes to
>>> jvmtiEnv.cpp could be removed or leave the whole code and change as
>>> is. Please let me know what you think?
>> 
>> 
>> In the interest of moving forward you can push what you have and I will file
>> a bug against CDS to sort out classLoaderExt.cpp.
>> 
> 
> Thanks! As the current version also passed the submit-repo tests I've pushed 
> it.
> 
> Regarding classLoaderExt.cpp, I think it is OK to exclude it for
> non-CDS builds. If my IDE doesn't cheat on me (see [1]),
> ClassLoaderExt is mostly used from other CDS-only files
> (classListParser.cpp, systemDictionaryShared.cpp, filemap.cpp,
> metaspaceShared.cpp). The only references from non-CDS files are from
> classLoader.cpp an jvmtiEnv.cpp. The ones from classLoader.cpp are all
> guarded with 'INCLUDE_CDS' or they only use functions defined in
> classLoaderExt.hpp. The single remaining reference from jvmtiEnv.cpp
> has been guarded with 'INCLUDE_CDS' by my change.
> 
> I think it is a matter of taste if we leave this as is or move the
> offending function from classLoaderExt.cpp to classLoaderExt.hpp and
> remove the new guard from jvmtiEnv.cpp.

For the classLoaderExt.cpp bug, we could use a private function, 
ClassLoaderExt::disable_shared_platform_and_app_classes, which does the logic 
in the original ClassLoaderExt::append_boot_classpath #INCLUDE_CDS. 
ClassLoaderExt::append_boot_classpath could be defined in classLoaderExt.hpp as:

void disable_shared_platform_and_app_classes() NOT_CDS_RETURN;

void append_boot_classpath(ClassPathEntry* new_entry) {
  disable_shared_platform_and_app_classes();
  ClassLoader::add_to_boot_append_entries(new_entry);
}

The new guard can be removed from jvmtiEnv.cpp with those. Reducing CDS 
specifics from general code probably is cleaner.

Thanks,
Jiangli


> 
> Regards,
> Volker
> 
> [1] http://cr.openjdk.java.net/~simonis/webrevs/2018/ClassLoaderExt.html
> 
>> Thanks,
>> David
>> 
>> 
>>> Regards,
>>> Volker
>>> 
 Thanks,
 David
 
 
 On 19/06/2018 2:04 AM, Volker Simonis wrote:
> 
> 
> On Mon, Jun 18, 2018 at 8:17 AM, David Holmes 
> wrote:
>> 
>> 
>> Hi Volker,
>> 
>> src/hotspot/share/runtime/globals.hpp
>> 
>> This change should not be needed! We do minimal VM builds without CDS
>> and
>> we
>> don't have to touch the UseSharedSpaces defaults (unless recent change
>> have
>> broken this - in which case that needs to be addressed in its own
>> right!)
>> 
> 
> Yes, you're right, CDS_ONLY/NOT_CDS isn't really required here,
> because UseSharedSpaces is reseted later on at the end of
> Arguments::parse(). I just thought it would be cleaner to disable it
> statically, if the VM doesn't support it. But anyway I don't really
> mind and I've reverted that change in globals.hpp.
> 
>> src/hotspot/share/classfile/javaClasses.cpp
>> 
>> AFAICS you should be using INCLUDE_CDS in the ifdefs not
>> INCLUDE_CDS_JAVA_HEAP. But again I'm unclear (as was Thomas) why this
>> should
>> be needed as we have not needed it before. As Thomas notes we have:
>> 
>> ./hotspot/share/memory/metaspaceShared.hpp:  static bool
>> is_archive_object(oop p) NOT_CDS_JAVA_HEAP_RETURN_(false);
>> ./hotspot/share/classfile/stringTable.hpp:  static oop
>> create_archived_string(oop s, Thread* THREAD)
>> NOT_CDS_JAVA_HEAP_RETURN_(NULL);
>> 
>> so these methods should be defined when CDS is not available.
>> 
> 
> Thomas and you are right. Must have been a mis-configuration on AIX
> where I saw undefined symbols at link time. I've removed the ifdefs
> from javaClasses.cpp now.
> 
> Finally, I've also wrapped all the FileMapInfo fields in vmStructs.cpp

Re: RFR(M): 8204965: Fix '--disable-cds' and disable CDS on AIX by default

[Jiangli Zhou]

Hi Volker,

Thanks for adding CDS_ONLY to all FileMapInfo fields. It looks cleaner also 
with Thomas and David’s suggestion to remove the macros in globals.hpp and 
javaClasses.cpp.

Thanks!
Jiangli

> On Jun 18, 2018, at 9:04 AM, Volker Simonis  wrote:
> 
> On Mon, Jun 18, 2018 at 8:17 AM, David Holmes  wrote:
>> Hi Volker,
>> 
>> src/hotspot/share/runtime/globals.hpp
>> 
>> This change should not be needed! We do minimal VM builds without CDS and we
>> don't have to touch the UseSharedSpaces defaults (unless recent change have
>> broken this - in which case that needs to be addressed in its own right!)
>> 
> 
> Yes, you're right, CDS_ONLY/NOT_CDS isn't really required here,
> because UseSharedSpaces is reseted later on at the end of
> Arguments::parse(). I just thought it would be cleaner to disable it
> statically, if the VM doesn't support it. But anyway I don't really
> mind and I've reverted that change in globals.hpp.
> 
>> src/hotspot/share/classfile/javaClasses.cpp
>> 
>> AFAICS you should be using INCLUDE_CDS in the ifdefs not
>> INCLUDE_CDS_JAVA_HEAP. But again I'm unclear (as was Thomas) why this should
>> be needed as we have not needed it before. As Thomas notes we have:
>> 
>> ./hotspot/share/memory/metaspaceShared.hpp:  static bool
>> is_archive_object(oop p) NOT_CDS_JAVA_HEAP_RETURN_(false);
>> ./hotspot/share/classfile/stringTable.hpp:  static oop
>> create_archived_string(oop s, Thread* THREAD)
>> NOT_CDS_JAVA_HEAP_RETURN_(NULL);
>> 
>> so these methods should be defined when CDS is not available.
>> 
> 
> Thomas and you are right. Must have been a mis-configuration on AIX
> where I saw undefined symbols at link time. I've removed the ifdefs
> from javaClasses.cpp now.
> 
> Finally, I've also wrapped all the FileMapInfo fields in vmStructs.cpp
> into CDS_ONLY macros as suggested by Jiangli because the really only
> make sense for a CDS-enabled VM.
> 
> Here's the new webrev:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965.v3/
> 
> Please let me know if you think there's still something missing.
> 
> Regards,
> Volker
> 
> 
>> ??
>> 
>> Thanks,
>> David
>> -
>> 
>> 
>> 
>> 
>> 
>> On 15/06/2018 12:26 AM, Volker Simonis wrote:
>>> 
>>> Hi,
>>> 
>>> can I please have a review for the following fix:
>>> 
>>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965/
>>> https://bugs.openjdk.java.net/browse/JDK-8204965
>>> 
>>> CDS does currently not work on AIX because of the way how we
>>> reserve/commit memory on AIX. The problem is that we're using a
>>> combination of shmat/mmap depending on the page size and the size of
>>> the memory chunk to reserve. This makes it impossible to reliably
>>> reserve the memory for the CDS archive and later on map the various
>>> parts of the archive into these regions.
>>> 
>>> In order to fix this we would have to completely rework the memory
>>> reserve/commit/uncommit logic on AIX which is currently out of our
>>> scope because of resource limitations.
>>> 
>>> Unfortunately, I could not simply disable CDS in the configure step
>>> because some of the shared code apparently relies on parts of the CDS
>>> code which gets excluded from the build when CDS is disabled. So I
>>> also fixed the offending parts in hotspot and cleaned up the configure
>>> logic for CDS.
>>> 
>>> Thank you and best regards,
>>> Volker
>>> 
>>> PS: I did run the job through the submit forest
>>> (mach5-one-simonis-JDK-8204965-20180613-1946-26349) but the results
>>> weren't really useful because they mention build failures on linux-x64
>>> which I can't reproduce locally.
>>> 
>> 

Re: RFR(M): 8204965: Fix '--disable-cds' and disable CDS on AIX by default

[Jiangli Zhou]

Hi Volker,

> On Jun 15, 2018, at 12:43 AM, Volker Simonis  wrote:
> 
> Hi Jiangli,
> 
> thanks for looking at the change.
> 
> 'CDS_only' is only required for static fields because the
> VMStructEntry for them contains a reference to the actual static field
> which isn't present if we disable CDS, because the corresponding
> compilations units (i.e. filemap.cpp) won't be part of libjvm.so. For
> non-static fields, the VMStructEntry structure only contains the
> offset of the corresponding field with regards to an object of that
> type which is harmless.

Thanks for the explanation. For consistency, would it be worth to add CDS_ONLY 
for the non-static fields in FileMapInfo also?

Thanks,
Jiangli

> 
> Regards,
> Volker
> 
> 
> On Thu, Jun 14, 2018 at 6:42 PM, Jiangli Zhou  wrote:
>> Hi Volker,
>> 
>> The changes look good to me overall. I’ll refer to the JVMTI experts for
>> jvmtiEnv.cpp change. I have a question for the change in vmStructs.cpp. Any
>> reason why only _current_info needs CDS_ONLY?
>> 
>>   //
>> \
>>   /* FileMapInfo fields (CDS archive related) */
>> \
>>   //
>> \
>> 
>> \
>>   nonstatic_field(FileMapInfo, _header,
>> FileMapInfo::FileMapHeader*)   \
>> - static_field(FileMapInfo, _current_info,
>> FileMapInfo*)  \
>> + CDS_ONLY(static_field(FileMapInfo,_current_info,
>> FileMapInfo*)) \
>>   nonstatic_field(FileMapInfo::FileMapHeader,  _space[0],
>> FileMapInfo::FileMapHeader::space_info)\
>>   nonstatic_field(FileMapInfo::FileMapHeader::space_info, _addr._base,
>> char*) \
>>   nonstatic_field(FileMapInfo::FileMapHeader::space_info, _used,
>> size_t)\
>> 
>> \
>> 
>> Thanks,
>> Jiangli
>> 
>> On Jun 14, 2018, at 7:26 AM, Volker Simonis 
>> wrote:
>> 
>> Hi,
>> 
>> can I please have a review for the following fix:
>> 
>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965/
>> https://bugs.openjdk.java.net/browse/JDK-8204965
>> 
>> CDS does currently not work on AIX because of the way how we
>> reserve/commit memory on AIX. The problem is that we're using a
>> combination of shmat/mmap depending on the page size and the size of
>> the memory chunk to reserve. This makes it impossible to reliably
>> reserve the memory for the CDS archive and later on map the various
>> parts of the archive into these regions.
>> 
>> In order to fix this we would have to completely rework the memory
>> reserve/commit/uncommit logic on AIX which is currently out of our
>> scope because of resource limitations.
>> 
>> Unfortunately, I could not simply disable CDS in the configure step
>> because some of the shared code apparently relies on parts of the CDS
>> code which gets excluded from the build when CDS is disabled. So I
>> also fixed the offending parts in hotspot and cleaned up the configure
>> logic for CDS.
>> 
>> Thank you and best regards,
>> Volker
>> 
>> PS: I did run the job through the submit forest
>> (mach5-one-simonis-JDK-8204965-20180613-1946-26349) but the results
>> weren't really useful because they mention build failures on linux-x64
>> which I can't reproduce locally.
>> 
>> 

Re: RFR(M): 8204965: Fix '--disable-cds' and disable CDS on AIX by default

[Jiangli Zhou]

Hi Volker,

The changes look good to me overall. I’ll refer to the JVMTI experts for 
jvmtiEnv.cpp change. I have a question for the change in vmStructs.cpp. Any 
reason why only _current_info needs CDS_ONLY?

   //   
  \
   /* FileMapInfo fields (CDS archive related) */   
  \
   //   
  \

  \
   nonstatic_field(FileMapInfo, _header,
   FileMapInfo::FileMapHeader*)   \
- static_field(FileMapInfo, _current_info,  
   FileMapInfo*)  \
+ CDS_ONLY(static_field(FileMapInfo,_current_info,  
   FileMapInfo*)) \
   nonstatic_field(FileMapInfo::FileMapHeader,  _space[0],  
   FileMapInfo::FileMapHeader::space_info)\
   nonstatic_field(FileMapInfo::FileMapHeader::space_info, _addr._base, 
   char*) \
   nonstatic_field(FileMapInfo::FileMapHeader::space_info, _used,   
   size_t)\

  \
Thanks,
Jiangli

> On Jun 14, 2018, at 7:26 AM, Volker Simonis  wrote:
> 
> Hi,
> 
> can I please have a review for the following fix:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965/
> https://bugs.openjdk.java.net/browse/JDK-8204965
> 
> CDS does currently not work on AIX because of the way how we
> reserve/commit memory on AIX. The problem is that we're using a
> combination of shmat/mmap depending on the page size and the size of
> the memory chunk to reserve. This makes it impossible to reliably
> reserve the memory for the CDS archive and later on map the various
> parts of the archive into these regions.
> 
> In order to fix this we would have to completely rework the memory
> reserve/commit/uncommit logic on AIX which is currently out of our
> scope because of resource limitations.
> 
> Unfortunately, I could not simply disable CDS in the configure step
> because some of the shared code apparently relies on parts of the CDS
> code which gets excluded from the build when CDS is disabled. So I
> also fixed the offending parts in hotspot and cleaned up the configure
> logic for CDS.
> 
> Thank you and best regards,
> Volker
> 
> PS: I did run the job through the submit forest
> (mach5-one-simonis-JDK-8204965-20180613-1946-26349) but the results
> weren't really useful because they mention build failures on linux-x64
> which I can't reproduce locally.

Re: RFR: 8199807 & 8202738: AppCDS performs overly restrictive path matching check

[Jiangli Zhou]

> On May 14, 2018, at 2:56 PM, Magnus Ihse Bursie 
> <magnus.ihse.bur...@oracle.com> wrote:
> 
> On 2018-05-11 23:33, Erik Joelsson wrote:
>> Hello,
>> 
>> For the build change, it's very undesirable to always have to relink libjvm 
>> on every incremental build. Such a change cannot be accepted.
>> 
>> I have a counter suggestion, which is still a bit of a hack, but it will 
>> cause vm_version.cpp to be recompiled (almost) every time libjvm.so needs to 
>> be relinked. The drawback is that compiling vm_version.cpp is now bound to 
>> happen absolutely last and so cannot happen in parallel with other 
>> compilations.
>> 
>> Webrev: http://cr.openjdk.java.net/~erikj/8202738/webrev.01/index.html
> 
> This looks as good as it can get for a simple fix, but I'd still like to get 
> on the record that I think the way we handle both __TIME__/__DATE__ in 
> hotspot, and ad-hoc version strings in general, is broken and leave much to 
> be desired.

+1 on the above (both the review and the usage of __TIME__/__DATE__).

Thanks!

Jiangli

> 
> /Magnus
> 
> 
> 
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202738
>> 
>> /Erik
>> 
>> 
>> On 2018-05-10 16:11, Jiangli Zhou wrote:
>>> Hi,
>>> 
>>> Please review the following webrev that addresses the issue of copied/moved 
>>> JDK image after generating a CDS archive. Thanks Karen Kinnear and Alan 
>>> Baterman for initiating the investigation & discussions in this area 
>>> (especially the ease of usage). Thanks Ioi for implementing a test case for 
>>> moved JDK (JDK-8202935).
>>> 
>>> webrev: http://cr.openjdk.java.net/~jiangli/8199807_8202738/webrev.00/
>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8199807?filter=14921
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202738?filter=14921
>>> 
>>> The webrev includes the following three main parts:
>>> 
>>> 1. Reduced check for JDK ‘modules’ image file at runtime. The runtime path 
>>> to the ‘modules’ image is no longer required to the the same as dump time 
>>> path. Runtime performs file size check only for the ‘modules’ image, which 
>>> must match with the dump time ‘modules’ size. Invalidation of an outdated 
>>> archive is achieved by the existing vm_version string check (the archived 
>>> vm_version string must match with the runtime vm_version string).
>>> 
>>> 2. Boot path check are now performed based on the content of the archive. 
>>> Also added a new test case in BootClassPathMismatch.java and add more 
>>> comments for existing test cases.
>>> 
>>> 3. Fixed the stale vm_version string issue with incremental build. The 
>>> issue was discovered during the work of 8199807. CDS uses vm_version string 
>>> as part of the runtime validation check for archive. A stale vm_version 
>>> string causes the CDS runtime to mistakenly accept an outdated archive. The 
>>> fix is to make sure vm_version.o is recompiled properly when the library/vm 
>>> is rebuilt.
>>> 
>>> Tested with hs-tier1-4 and jdk-tier1-2. Tested by relocating the JDK image 
>>> manually after generating an archive. Also tested with Ioi’s test both 
>>> locally and via Mach5.
>>> 
>>> Thanks,
>>> Jiangli
>>> 
>> 
> 

Re: RFR: 8199807 & 8202738: AppCDS performs overly restrictive path matching check

[Jiangli Zhou]

Hi Erik,

Thank you so much for investigating a proper fix for the vm_version.o issue. I 
will withdraw the build change from my original webrev.

Thanks again for taking over the bug!
Jiangli 

> On May 11, 2018, at 2:33 PM, Erik Joelsson <erik.joels...@oracle.com> wrote:
> 
> Received: from [10.132.185.167] (/10.132.185.167)
>by default (Oracle Beehive Gateway v4.0)
>with ESMTP ; Fri, 11 May 2018 14:33:20 -0700
> Subject: Re: RFR: 8199807 & 8202738: AppCDS performs overly restrictive path
> matching check
> To: Jiangli Zhou <jiangli.z...@oracle.com>,
> "hotspot-runtime-...@openjdk.java.net runtime"
> <hotspot-runtime-...@openjdk.java.net>,
> build-dev <build-dev@openjdk.java.net>
> References: <b173b2ef-9908-418a-9dcc-ee6d9133d...@oracle.com>
> From: Erik Joelsson <erik.joels...@oracle.com>
> Organization: Oracle Corporation
> Message-ID: <ae4c66a1-ad7d-00f1-ced1-729704d56...@oracle.com>
> Date: Fri, 11 May 2018 14:33:20 -0700
> User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0)
> Gecko/20100101 Thunderbird/52.7.0
> MIME-Version: 1.0
> In-Reply-To: <b173b2ef-9908-418a-9dcc-ee6d9133d...@oracle.com>
> Content-Type: text/plain; charset=utf-8; format=flowed
> Content-Transfer-Encoding: 8bit
> Content-Language: en-US
> 
> Hello,
> 
> For the build change, it's very undesirable to always have to relink libjvm 
> on every incremental build. Such a change cannot be accepted.
> 
> I have a counter suggestion, which is still a bit of a hack, but it will 
> cause vm_version.cpp to be recompiled (almost) every time libjvm.so needs to 
> be relinked. The drawback is that compiling vm_version.cpp is now bound to 
> happen absolutely last and so cannot happen in parallel with other 
> compilations.
> 
> Webrev: http://cr.openjdk.java.net/~erikj/8202738/webrev.01/index.html
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8202738
> 
> /Erik
> 
> 
>> On 2018-05-10 16:11, Jiangli Zhou wrote:
>> Hi,
>> 
>> Please review the following webrev that addresses the issue of copied/moved 
>> JDK image after generating a CDS archive. Thanks Karen Kinnear and Alan 
>> Baterman for initiating the investigation & discussions in this area 
>> (especially the ease of usage). Thanks Ioi for implementing a test case for 
>> moved JDK (JDK-8202935).
>> 
>> webrev: http://cr.openjdk.java.net/~jiangli/8199807_8202738/webrev.00/
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8199807?filter=14921
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202738?filter=14921
>> 
>> The webrev includes the following three main parts:
>> 
>> 1. Reduced check for JDK ‘modules’ image file at runtime. The runtime path 
>> to the ‘modules’ image is no longer required to the the same as dump time 
>> path. Runtime performs file size check only for the ‘modules’ image, which 
>> must match with the dump time ‘modules’ size. Invalidation of an outdated 
>> archive is achieved by the existing vm_version string check (the archived 
>> vm_version string must match with the runtime vm_version string).
>> 
>> 2. Boot path check are now performed based on the content of the archive. 
>> Also added a new test case in BootClassPathMismatch.java and add more 
>> comments for existing test cases.
>> 
>> 3. Fixed the stale vm_version string issue with incremental build. The issue 
>> was discovered during the work of 8199807. CDS uses vm_version string as 
>> part of the runtime validation check for archive. A stale vm_version string 
>> causes the CDS runtime to mistakenly accept an outdated archive. The fix is 
>> to make sure vm_version.o is recompiled properly when the library/vm is 
>> rebuilt.
>> 
>> Tested with hs-tier1-4 and jdk-tier1-2. Tested by relocating the JDK image 
>> manually after generating an archive. Also tested with Ioi’s test both 
>> locally and via Mach5.
>> 
>> Thanks,
>> Jiangli
>> 
> 
> Hello,
> 
> For the build change, it's very undesirable to always have to relink libjvm 
> on every incremental build. Such a change cannot be accepted.
> 
> I have a counter suggestion, which is still a bit of a hack, but it will 
> cause vm_version.cpp to be recompiled (almost) every time libjvm.so needs to 
> be relinked. The drawback is that compiling vm_version.cpp is now bound to 
> happen absolutely last and so cannot happen in parallel with other 
> compilations.
> 
> Webrev: http://cr.openjdk.java.net/~erikj/8202738/webrev.01/index.html
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8202738
> 
> /Erik
> 
> 
>> On 2018-05-10 16:11,

RFR: 8199807 & 8202738: AppCDS performs overly restrictive path matching check

[Jiangli Zhou]

Hi,

Please review the following webrev that addresses the issue of copied/moved JDK 
image after generating a CDS archive. Thanks Karen Kinnear and Alan Baterman 
for initiating the investigation & discussions in this area (especially the 
ease of usage). Thanks Ioi for implementing a test case for moved JDK 
(JDK-8202935).

webrev: http://cr.openjdk.java.net/~jiangli/8199807_8202738/webrev.00/
RFE: https://bugs.openjdk.java.net/browse/JDK-8199807?filter=14921
Bug: https://bugs.openjdk.java.net/browse/JDK-8202738?filter=14921

The webrev includes the following three main parts:

1. Reduced check for JDK ‘modules’ image file at runtime. The runtime path to 
the ‘modules’ image is no longer required to the the same as dump time path. 
Runtime performs file size check only for the ‘modules’ image, which must match 
with the dump time ‘modules’ size. Invalidation of an outdated archive is 
achieved by the existing vm_version string check (the archived vm_version 
string must match with the runtime vm_version string).

2. Boot path check are now performed based on the content of the archive. Also 
added a new test case in BootClassPathMismatch.java and add more comments for 
existing test cases.

3. Fixed the stale vm_version string issue with incremental build. The issue 
was discovered during the work of 8199807. CDS uses vm_version string as part 
of the runtime validation check for archive. A stale vm_version string causes 
the CDS runtime to mistakenly accept an outdated archive. The fix is to make 
sure vm_version.o is recompiled properly when the library/vm is rebuilt.

Tested with hs-tier1-4 and jdk-tier1-2. Tested by relocating the JDK image 
manually after generating an archive. Also tested with Ioi’s test both locally 
and via Mach5.

Thanks,
Jiangli

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

> On Apr 27, 2018, at 1:43 AM, David Holmes <david.hol...@oracle.com> wrote:
> 
> On 27/04/2018 4:20 AM, Jiangli Zhou wrote:
>>> On Apr 25, 2018, at 10:09 PM, David Holmes <david.hol...@oracle.com> wrote:
>>> 
>>> On 26/04/2018 2: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!
>>> 
>>> One additional comment on 
>>> test/hotspot/jtreg/runtime/appcds/SharedArchiveFile.java - it seems 
>>> insufficient to just check the output for the word "dump". I would expect 
>>> to see something more definitively associated with -Xshare:dump and also a 
>>> check that it exited cleanly (in case we find "core dump”).
> 
> I should have of course said "the word Dumping" and "in case we find 'Dumping 
> core'". :)
> 
>> With other test cases being removed from appcds/SharedArchiveFile.java, I 
>> just realized that the test now essentially is the same as the 
>> jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java test. 
>> SharedArchiveFile/SharedArchiveFile.java includes more robust checks as you 
>> suggested and also tests runtime. Your comments above (and fresh morning 
>> coffee) reminded me that. :-) So I went ahead removed the 
>> appcds/SharedArchiveFile.java. Please let me know if you want to see the new 
>> webrev.
> 
> No that's fine. Removing the file completely makes sense.

Thanks!

Jiangli

> 
> Thanks,
> David
> 
>> Thanks!
>> Jiangli
>>> 
>>> Thanks,
>>> David
>>> -
>>> 
>>>> - Filed JDK-8202282 (https://bugs.openjdk.java.net/browse/JDK-8202282) for 
>>>> TestCommon.makeCommandLineForAppCDS() cleanup.
>>>> - Removed case 2, 3 and 4 in SharedArchiveFile.java.
>>>> - Removed UseAppCDS.java test.
>>>> - 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
>>>> 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/
>>>&g

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

> 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

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

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?

>> - 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 testin

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

Thanks, Magnus and Erik!

Jiangli

> On Apr 26, 2018, at 8:55 AM, Erik Joelsson <erik.joels...@oracle.com> wrote:
> 
> Build looks good, thanks.
> 
> /Erik
> 
> 
> On 2018-04-25 21:34, 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/
>> 
>> - Filed JDK-8202282 (https://bugs.openjdk.java.net/browse/JDK-8202282) for 
>> TestCommon.makeCommandLineForAppCDS() cleanup.
>> - Removed case 2, 3 and 4 in SharedArchiveFile.java.
>> - Removed UseAppCDS.java test.
>> - 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
>> 
>> 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.
>>>&g

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

> On Apr 25, 2018, at 10:09 PM, David Holmes <david.hol...@oracle.com> wrote:
> 
> On 26/04/2018 2: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!

> 
> One additional comment on 
> test/hotspot/jtreg/runtime/appcds/SharedArchiveFile.java - it seems 
> insufficient to just check the output for the word "dump". I would expect to 
> see something more definitively associated with -Xshare:dump and also a check 
> that it exited cleanly (in case we find "core dump”).

With other test cases being removed from appcds/SharedArchiveFile.java, I just 
realized that the test now essentially is the same as the 
jtreg/runtime/SharedArchiveFile/SharedArchiveFile.java test. 
SharedArchiveFile/SharedArchiveFile.java includes more robust checks as you 
suggested and also tests runtime. Your comments above (and fresh morning 
coffee) reminded me that. :-) So I went ahead removed the 
appcds/SharedArchiveFile.java. Please let me know if you want to see the new 
webrev.

Thanks!
Jiangli

> 
> Thanks,
> David
> -
> 
>> - Filed JDK-8202282 (https://bugs.openjdk.java.net/browse/JDK-8202282) for 
>> TestCommon.makeCommandLineForAppCDS() cleanup.
>> - Removed case 2, 3 and 4 in SharedArchiveFile.java.
>> - Removed UseAppCDS.java test.
>> - 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
>> 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 ne

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

Here is the incremental webrev with updates that incorporate all feedbacks:
http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev_inc.02/

- Filed JDK-8202282 (https://bugs.openjdk.java.net/browse/JDK-8202282) for 
TestCommon.makeCommandLineForAppCDS() cleanup.
- Removed case 2, 3 and 4 in SharedArchiveFile.java.
- Removed UseAppCDS.java test.
- 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

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.
>>> Sou

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

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 
>>

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

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 
>>>>

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

Hi Magnus,

Thanks a lot for the review!

Jiangli

> On Apr 25, 2018, at 2:17 AM, Magnus Ihse Bursie 
> <magnus.ihse.bur...@oracle.com> 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 cl

Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete

[Jiangli Zhou]

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.

> 
> 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

Re: RFR: compile-time guard some UseSharedSpaces-only coding with the INCLUDE_CDS macro - was :RE: jdk10 : UseSharedSpaces flag and INCLUDE_CDS macro

[Jiangli Zhou]

Hi Matthias,

> On Jul 18, 2017, at 12:24 AM, Baesken, Matthias <matthias.baes...@sap.com> 
> wrote:
> 
> Hi , thanks for the review .
>  
>  
> >Related to the change in universe_init() (universe.cpp), the 'if 
> >(DumpSharedSpaces)’ block at line 715 is also CDS only.
> >To be consistent, you might want to include it under #if INCLUDE_CDS as well.
>  
> I  added the suggested  change and  created a second webrev :
>  
> http://cr.openjdk.java.net/~mbaesken/webrevs/8184323.1/ 
> <http://cr.openjdk.java.net/~mbaesken/webrevs/8184323.1/>
Looks good. I see Coleen already said she will push the change for you.

Thanks,
Jiangli

>  
>  
> Is a second review needed ?
>  
>  
> Best regards, Matthias
>  
> From: Jiangli Zhou [mailto:jiangli.z...@oracle.com 
> <mailto:jiangli.z...@oracle.com>] 
> Sent: Samstag, 15. Juli 2017 06:37
> To: Baesken, Matthias <matthias.baes...@sap.com 
> <mailto:matthias.baes...@sap.com>>
> Cc: hotspot-...@openjdk.java.net <mailto:hotspot-...@openjdk.java.net>; 
> build-dev@openjdk.java.net <mailto:build-dev@openjdk.java.net>
> Subject: Re: RFR: compile-time guard some UseSharedSpaces-only coding with 
> the INCLUDE_CDS macro - was :RE: jdk10 : UseSharedSpaces flag and INCLUDE_CDS 
> macro
>  
> Hi Baesken,
>  
> Thank you for making the changes.
>  
> Related to the change in universe_init() (universe.cpp), the 'if 
> (DumpSharedSpaces)’ block at line 715 is also CDS only. To be consistent, you 
> might want to include it under #if INCLUDE_CDS as well.
> --- 695,716 
> Universe::_loader_addClass_cache= new LatestMethodCache();
> Universe::_pd_implies_cache = new LatestMethodCache();
> Universe::_throw_illegal_access_error_cache = new LatestMethodCache();
> Universe::_do_stack_walk_cache = new LatestMethodCache();
>   
> + #if INCLUDE_CDS
> if (UseSharedSpaces) {
>   // Read the data structures supporting the shared spaces (shared
>   // system dictionary, symbol table, etc.).  After that, access to
>   // the file (other than the mapped regions) is no longer needed, and
>   // the file is closed. Closing the file does not affect the
>   // currently mapped regions.
>   MetaspaceShared::initialize_shared_spaces();
>   StringTable::create_table();
> !   } else
> ! #endif
> !   {
>   SymbolTable::create_table();
>   StringTable::create_table();
>   
>   if (DumpSharedSpaces) {
> MetaspaceShared::prepare_for_dumping();
> In the past we have been trying to avoid adding too many #if in the code for 
> better readability. For the CDS only code in universe.cpp (and a few other 
> files), since the callee functions (e.g. initialize_shared_spaces()) are 
> already defined with two versions (with and without INCLUDE_CDS). The builds 
> without CDS also works fine even #if INCLUDE_CDS are not added.
>  
> Thanks,
> Jiangli
>  
> On Jul 13, 2017, at 5:18 AM, Baesken, Matthias <matthias.baes...@sap.com 
> <mailto:matthias.baes...@sap.com>> wrote:
>  
> Thank you for noticing that. Yes, it would be helpful if you can add the #if 
> INCLUDE_CDS for CDS only code. 
> I can help you integrate the changes after they are reviewed.
> 
> Thanks for your feedback !
> 
> I created a bug :
> 
> https://bugs.openjdk.java.net/browse/JDK-8184323 
> <https://bugs.openjdk.java.net/browse/JDK-8184323>
> 
> 
> and a webrev :
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8184323/ 
> <http://cr.openjdk.java.net/~mbaesken/webrevs/8184323/>
> 
> 
> Best regards, Matthias
> 
> 
> -Original Message-
> From: Jiangli Zhou [mailto:jiangli.z...@oracle.com 
> <mailto:jiangli.z...@oracle.com>] 
> Sent: Montag, 10. Juli 2017 17:54
> To: Baesken, Matthias <matthias.baes...@sap.com 
> <mailto:matthias.baes...@sap.com>>
> Cc: hotspot-...@openjdk.java.net <mailto:hotspot-...@openjdk.java.net>; 
> build-dev@openjdk.java.net <mailto:build-dev@openjdk.java.net>
> Subject: Re: jdk10 : UseSharedSpaces flag and INCLUDE_CDS macro
> 
> Hi Matthias,
> 
> Thank you for noticing that. Yes, it would be helpful if you can add the #if 
> INCLUDE_CDS for CDS only code. I can help you integrate the changes after 
> they are reviewed.
> 
> Thanks!
> Jiangli
> 
> 
> On Jul 5, 2017, at 6:36 AM, Baesken, Matthias <matthias.baes...@sap.com 
> <mailto:matthias.baes...@sap.com>> wrote:
> 
> Hello, when looking into CDS related build options,  I noticed  that most  
> code-parts  that are executed only when UseSharedSpaces is set,
> are guarded by  the compile-time macroINCLUDE_CDS  

Re: RFR: compile-time guard some UseSharedSpaces-only coding with the INCLUDE_CDS macro - was :RE: jdk10 : UseSharedSpaces flag and INCLUDE_CDS macro

[Jiangli Zhou]

Hi Baesken,

Thank you for making the changes.

Related to the change in universe_init() (universe.cpp), the 'if 
(DumpSharedSpaces)’ block at line 715 is also CDS only. To be consistent, you 
might want to include it under #if INCLUDE_CDS as well.
--- 695,716 
Universe::_loader_addClass_cache= new LatestMethodCache();
Universe::_pd_implies_cache = new LatestMethodCache();
Universe::_throw_illegal_access_error_cache = new LatestMethodCache();
Universe::_do_stack_walk_cache = new LatestMethodCache();
  
+ #if INCLUDE_CDS
if (UseSharedSpaces) {
  // Read the data structures supporting the shared spaces (shared
  // system dictionary, symbol table, etc.).  After that, access to
  // the file (other than the mapped regions) is no longer needed, and
  // the file is closed. Closing the file does not affect the
  // currently mapped regions.
  MetaspaceShared::initialize_shared_spaces();
  StringTable::create_table();
!   } else
! #endif
!   {
  SymbolTable::create_table();
  StringTable::create_table();
  
  if (DumpSharedSpaces) {
MetaspaceShared::prepare_for_dumping();
In the past we have been trying to avoid adding too many #if in the code for 
better readability. For the CDS only code in universe.cpp (and a few other 
files), since the callee functions (e.g. initialize_shared_spaces()) are 
already defined with two versions (with and without INCLUDE_CDS). The builds 
without CDS also works fine even #if INCLUDE_CDS are not added.

Thanks,
Jiangli

> On Jul 13, 2017, at 5:18 AM, Baesken, Matthias <matthias.baes...@sap.com> 
> wrote:
> 
>> Thank you for noticing that. Yes, it would be helpful if you can add the #if 
>> INCLUDE_CDS for CDS only code. 
>> I can help you integrate the changes after they are reviewed.
> 
> Thanks for your feedback !
> 
> I created a bug :
> 
> https://bugs.openjdk.java.net/browse/JDK-8184323
> 
> 
> and a webrev :
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8184323/
> 
> 
> Best regards, Matthias
> 
> 
> -Original Message-
> From: Jiangli Zhou [mailto:jiangli.z...@oracle.com] 
> Sent: Montag, 10. Juli 2017 17:54
> To: Baesken, Matthias <matthias.baes...@sap.com>
> Cc: hotspot-...@openjdk.java.net; build-dev@openjdk.java.net
> Subject: Re: jdk10 : UseSharedSpaces flag and INCLUDE_CDS macro
> 
> Hi Matthias,
> 
> Thank you for noticing that. Yes, it would be helpful if you can add the #if 
> INCLUDE_CDS for CDS only code. I can help you integrate the changes after 
> they are reviewed.
> 
> Thanks!
> Jiangli
> 
>> On Jul 5, 2017, at 6:36 AM, Baesken, Matthias <matthias.baes...@sap.com> 
>> wrote:
>> 
>> Hello, when looking into CDS related build options,  I noticed  that most  
>> code-parts  that are executed only when UseSharedSpaces is set,
>> are guarded by  the compile-time macroINCLUDE_CDS   to support  
>> switching off   compilation of this coding
>> when  CDS is disabled at compile time  :
>> 
>> 
>> See  hotspot/make/lib/JvmFeatures.gmk  :
>> 
>> ifneq ($(call check-jvm-feature, cds), true)
>> JVM_CFLAGS_FEATURES += -DINCLUDE_CDS=0
>> 
>> 
>> 
>> However some places miss the compile-time guarding ( #if INCLUDE_CDS  ) 
>> for example in
>> 
>> 
>> share/vm/prims/jvmtiRedefineClasses.cpp
>> share/vm/memory/universe.cpp
>> 
>> (also some other places)
>> 
>> 
>> Should I prepare a change and add the compile-time guard at the places where 
>> missing as well ?
>> 
>> Best regards, Matthias
> 

Re: jdk10 : UseSharedSpaces flag and INCLUDE_CDS macro

[Jiangli Zhou]

Hi Matthias,

Thank you for noticing that. Yes, it would be helpful if you can add the #if 
INCLUDE_CDS for CDS only code. I can help you integrate the changes after they 
are reviewed.

Thanks!
Jiangli

> On Jul 5, 2017, at 6:36 AM, Baesken, Matthias  
> wrote:
> 
> Hello, when looking into CDS related build options,  I noticed  that most  
> code-parts  that are executed only when UseSharedSpaces is set,
>  are guarded by  the compile-time macroINCLUDE_CDS   to support  
> switching off   compilation of this coding
>  when  CDS is disabled at compile time  :
> 
> 
> See  hotspot/make/lib/JvmFeatures.gmk  :
> 
> ifneq ($(call check-jvm-feature, cds), true)
>  JVM_CFLAGS_FEATURES += -DINCLUDE_CDS=0
> 
> 
> 
> However some places miss the compile-time guarding ( #if INCLUDE_CDS  ) 
> for example in
> 
> 
> share/vm/prims/jvmtiRedefineClasses.cpp
> share/vm/memory/universe.cpp
> 
> (also some other places)
> 
> 
> Should I prepare a change and add the compile-time guard at the places where 
> missing as well ?
> 
> Best regards, Matthias