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