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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad
Hi,

Jiangli and I discussed a subtle issue with the archiving changes here
last week (she's currently on vacation, and I was at JVMLS this week, so
it was never brought to the list, sorry!)

Basically: the empty configuration uses things like
Collections.emptySet(), which means it will end up deserializing an
object from the archive that doesn't (necessarily) have the same
identity as the object retrieved from direct uses of Collection.emptySet().

Same goes for the Set.of()/Map.of()/List.of() singletons which might now
already exist at runtime in two versions since the change to archive
ModuleDescriptors. Oops!

This glitch is probably fine semantically for most intents and purposes,
but could lead to some unnecessary inefficiencies and theoretically real
surprises/bugs, so we probably need to ensure that all singletons we
serialize retain the property that they are identical to the object a
caller of the respective getter would receive.

I think this can be deferred to a must-fix follow-up, mainly since
there's some benefit in first cleaning up a few things here first:

- currently we have uses of both Collections.emptyFoo() and Foo.of() in
the ModuleDescriptor/Configuration code

- to be correct, we thus need to archive the Collections.emptyFoo()
singletons *and* the various java.util.ImmutableCollections ones

- if we first consolidated all uses of Collections.emptyFoo() to
Foo.of() (in java.lang.module), we'd only need to archive the immutable
singletons in java.util.ImmutableCollections

Consolidating to Foo.of() was actually already being considered anyway,
so I'll go ahead with a separate RFE..

Thanks!

/Claes


On 2018-07-20 20:31, Jiangli Zhou wrote:
>
>
> Many thanks to Alan and Claes for discussions and contributions to
> this change!
>
> Webrev: http://cr.openjdk.java.net/~jiangli/8207263/webrev.00/

Reply | Threaded
Open this post in threaded view
|

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

Jiangli Zhou
Hi Claes,

Thanks for filing JDK-8209003
(https://bugs.openjdk.java.net/browse/JDK-8209003)! I'll look into
archiving the immutable singletons in ImmutableCollections.

Thanks again!!

Jiangli


On 8/4/18 9:22 AM, Claes Redestad wrote:

> Hi,
>
> Jiangli and I discussed a subtle issue with the archiving changes here
> last week (she's currently on vacation, and I was at JVMLS this week,
> so it was never brought to the list, sorry!)
>
> Basically: the empty configuration uses things like
> Collections.emptySet(), which means it will end up deserializing an
> object from the archive that doesn't (necessarily) have the same
> identity as the object retrieved from direct uses of
> Collection.emptySet().
>
> Same goes for the Set.of()/Map.of()/List.of() singletons which might
> now already exist at runtime in two versions since the change to
> archive ModuleDescriptors. Oops!
>
> This glitch is probably fine semantically for most intents and
> purposes, but could lead to some unnecessary inefficiencies and
> theoretically real surprises/bugs, so we probably need to ensure that
> all singletons we serialize retain the property that they are
> identical to the object a caller of the respective getter would receive.
>
> I think this can be deferred to a must-fix follow-up, mainly since
> there's some benefit in first cleaning up a few things here first:
>
> - currently we have uses of both Collections.emptyFoo() and Foo.of()
> in the ModuleDescriptor/Configuration code
>
> - to be correct, we thus need to archive the Collections.emptyFoo()
> singletons *and* the various java.util.ImmutableCollections ones
>
> - if we first consolidated all uses of Collections.emptyFoo() to
> Foo.of() (in java.lang.module), we'd only need to archive the
> immutable singletons in java.util.ImmutableCollections
>
> Consolidating to Foo.of() was actually already being considered
> anyway, so I'll go ahead with a separate RFE..
>
> Thanks!
>
> /Claes
>
>
> On 2018-07-20 20:31, Jiangli Zhou wrote:
>>
>>
>> Many thanks to Alan and Claes for discussions and contributions to
>> this change!
>>
>> Webrev: http://cr.openjdk.java.net/~jiangli/8207263/webrev.00/
>

Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman
In reply to this post by Claes Redestad
On 04/08/2018 17:22, Claes Redestad wrote:

> Hi,
>
> Jiangli and I discussed a subtle issue with the archiving changes here
> last week (she's currently on vacation, and I was at JVMLS this week,
> so it was never brought to the list, sorry!)
>
> Basically: the empty configuration uses things like
> Collections.emptySet(), which means it will end up deserializing an
> object from the archive that doesn't (necessarily) have the same
> identity as the object retrieved from direct uses of
> Collection.emptySet().
>
> Same goes for the Set.of()/Map.of()/List.of() singletons which might
> now already exist at runtime in two versions since the change to
> archive ModuleDescriptors. Oops!
>
> This glitch is probably fine semantically for most intents and
> purposes, but could lead to some unnecessary inefficiencies and
> theoretically real surprises/bugs, so we probably need to ensure that
> all singletons we serialize retain the property that they are
> identical to the object a caller of the respective getter would receive.
>
> I think this can be deferred to a must-fix follow-up, mainly since
> there's some benefit in first cleaning up a few things here first:
>
> - currently we have uses of both Collections.emptyFoo() and Foo.of()
> in the ModuleDescriptor/Configuration code
>
> - to be correct, we thus need to archive the Collections.emptyFoo()
> singletons *and* the various java.util.ImmutableCollections ones
>
> - if we first consolidated all uses of Collections.emptyFoo() to
> Foo.of() (in java.lang.module), we'd only need to archive the
> immutable singletons in java.util.ImmutableCollections
It's good to bring this up. I don't think there is a real issue right
now but it is a lurking hazard. As I recall, we started out in
java.lang.module long before the immutable collections were added so
this is why it has been using Collections.emptyXXX(). There shouldn't be
any issues changing all these usages to XXX.of().

A discussion for core-libs-dev but I wonder if we could get to the point
where emptyXXX() and XXX.of() returns the same empty collection so that
we don't have any issues elsewhere.

-Alan
Reply | Threaded
Open this post in threaded view
|

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

Jiangli Zhou
In reply to this post by Claes Redestad
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
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

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

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

Reply | Threaded
Open this post in threaded view
|

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

Jiangli Zhou
Thanks, Claes!

Jiangli


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

Reply | Threaded
Open this post in threaded view
|

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

Ioi Lam
In reply to this post by Claes Redestad
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
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Jiangli Zhou
Thanks, Ioi!

Jiangli


On 8/9/18 4:06 PM, Ioi Lam wrote:

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

Reply | Threaded
Open this post in threaded view
|

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

Calvin Cheung
In reply to this post by Jiangli Zhou
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
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Jiangli Zhou
Thanks, Calvin!

Jiangli


On 8/9/18 4:26 PM, Calvin Cheung wrote:

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