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
