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

[Magnus Ihse Bursie]

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