RFR: 8207263: Store the Configuration for system modules into CDS archive

[Calvin Cheung]

Looks good.

thanks,
Calvin

On 8/9/18, 11:42 AM, Jiangli Zhou wrote:
> Here is the updated webrev with comment changes suggested by Claes:
>
> http://cr.openjdk.java.net/~jiangli/8207263/webrev.02/
>
> Updated files:
> http://cr.openjdk.java.net/~jiangli/8207263/webrev.02/src/java.base/share/classes/java/util/ImmutableCollections.java.frames.html 
>
> http://cr.openjdk.java.net/~jiangli/8207263/webrev.02/src/java.base/share/classes/jdk/internal/module/ArchivedModuleGraph.java.frames.html 
>
>
> Thanks,
> Jiangli
>
> On 8/9/18 8:50 AM, Jiangli Zhou wrote:
>> Hi Claes,
>>
>> Thanks for reviewing the library and test changes! I'll remove the 
>> comments that you pointed below from ImmutableCollections and update 
>> the CheckArchivedModule test.
>>
>> Thanks!
>>
>> Jiangli
>>
>>
>> On 8/9/18 2:55 AM, Claes Redestad wrote:
>>> Library and test changes look good to me. Nits:
>>>
>>> ImmutableCollections:
>>>
>>> - // Initialize EMPTY_FOO from the archive comments feel redundant
>>>
>>> CheckArchivedModule:
>>>
>>> - perhaps call out that checking identity is necessary and intentional
>>>
>>> /Claes
>>>
>>> On 2018-08-09 01:43, Jiangli Zhou wrote:
>>>> Thanks Claes for resolving JDK-8209003 quickly. Here is my updated 
>>>> weberv that includes following changes:
>>>>
>>>> - Added archiving for singletons in ImmutableCollections 
>>>> (ListN.EMPTY_LIST, SetN.EMPTY_SET and MapN.EMPTY_MAP). Please see 
>>>> changes in ImmutableCollections.java.
>>>> - Incorporated Calvin and Ioi feedbacks on vmSymbols.hpp.
>>>> - Added a new test case to check the parents and modules of 
>>>> archived EMPTY_CONFIGURATION.
>>>> - Added argument length check in CheckArchivedModuleApp.java test 
>>>> as Calvin suggested.
>>>>
>>>> http://cr.openjdk.java.net/~jiangli/8207263/webrev.01/
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>> On 8/6/18 9:48 AM, Jiangli Zhou wrote:
>>>>> Hi Calvin and Ioi,
>>>>>
>>>>> Thanks for reviewing the change! I'll incorporate your suggestions.
>>>>>
>>>>> As Claes pointed out in his email, there was a subtle issue with 
>>>>> the empty configuration, which was undetected by the testing for 
>>>>> archiving changes but could introduce a bug in certain cases. 
>>>>> Claes has already filed JDK-8209003 for consolidating empty 
>>>>> collections usage in module code. I'll look into archiving the 
>>>>> immutable singletons in java.util.ImmutableCollections.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Jiangli
>>>>>
>>>>>
>>>>> On 8/3/18 2:58 PM, Ioi Lam wrote:
>>>>>> On 8/3/18 11:37 AM, Calvin Cheung wrote:
>>>>>>> Hi Jiangli,
>>>>>>>
>>>>>>> The changes look good to me.
>>>>>>>
>>>>>>> I have couple of minor comments:
>>>>>>>
>>>>>>> 1) vmSymbols.hpp
>>>>>>>
>>>>>>>  653   template(url_void_signature, "(Ljava/net/URL;)V") \
>>>>>>>  654   template(toFileURL_name, "toFileURL") \
>>>>>>>  655   template(toFileURL_signature, 
>>>>>>> "(Ljava/lang/String;)Ljava/net/URL;")
>>>>>>>
>>>>>>> Since you've moved the above lines to after 
>>>>>>> “template(systemModules_signature, …”, I’d suggest rearrange the 
>>>>>>> entire block (lines 652 - 659) in alphabetical order.
>>>>>>>
>>>>>> Hi Jiangli,
>>>>>>
>>>>>> I've reviewed the code. It looks like a clean change and it's 
>>>>>> great to make further progress in start-up improvement!
>>>>>>
>>>>>> Just a small note on vmSymbols.hpp:  this line can be deleted 
>>>>>> because the symbol is no longer used.
>>>>>>
>>>>>>   template(jdk_vm_cds_SharedClassInfo, "jdk/vm/cds/SharedClassInfo")
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>>> 2) CheckArchivedModuleApp.java
>>>>>>>
>>>>>>> Since it now expects two input args, I’d suggest checking the 
>>>>>>> number of input args and throw an exception if it is not equal 
>>>>>>> to two.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>>>
>>>>>>> On 7/20/18, 11:31 AM, Jiangli Zhou wrote:
>>>>>>>> Please review the following webrev that archives the system 
>>>>>>>> module boot layer Configuration (including all java objects 
>>>>>>>> reachable from the Configuration) in CDS archive. This is built 
>>>>>>>> on top of the earlier change for JDK-8202035 
>>>>>>>> (https://bugs.openjdk.java.net/browse/JDK-8202035), which 
>>>>>>>> provides a framework for object sub-graph archiving.
>>>>>>>>
>>>>>>>> The boot layer Configuration is created in 
>>>>>>>> ModuleBootstrap.boot() (similar to the archived system 
>>>>>>>> ModuleDescriptor objects, etc) and is unchanged after 
>>>>>>>> construction. With archived boot layer Configuration, it allows 
>>>>>>>> runtime to bypass the work for creating the configuration. 
>>>>>>>> Currently, this is only supported when the initial module is 
>>>>>>>> unnamed module. Measurements indicate archiving the boot layer 
>>>>>>>> Configuration improves the startup time by 1% ~ 1.5% (on 
>>>>>>>> linux-x64) when running HelloWorld from -cp at runtime.
>>>>>>>>
>>>>>>>> Many thanks to Alan and Claes for discussions and contributions 
>>>>>>>> to this change!
>>>>>>>>
>>>>>>>> Webrev: http://cr.openjdk.java.net/~jiangli/8207263/webrev.00/
>>>>>>>>
>>>>>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8207263
>>>>>>>>
>>>>>>>> Tested with tier1 - tier5 tests via mach5.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Jiangli
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>