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