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

[Ioi Lam]

Looks good to me, too. Thanks!

- Ioi


On 8/9/18 12:13 PM, Claes Redestad wrote:
> Updates looks good, thanks!
>
> /Claes
>
> On 2018-08-09 20:42, 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
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>