Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
Thanks a lot for reviewing, Mandy! Jiangli > On Jul 6, 2018, at 1:40 PM, mandy chung wrote: > > Hi Jiangli, > > On 6/28/18 4:15 PM, Jiangli Zhou wrote:> webrev: > http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/ >> RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921 > > Good work. I'm glad to see a pretty good startup improvement. > > I reviewed java.base change that looks good. > > Mandy
Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
> On Jul 6, 2018, at 1:36 PM, Ioi Lam wrote:
>
> Hi Jiangli,
>
> The VM changes look good to me.
Thanks!
>
> For the tests: I think we need a comment here saying that "mods" is
> intentionally empty, and also an explanation why it's not necessary to
> actually fill with actual modules?
Will do.
Thanks,
Jiangli
>
> Thanks
>
> - Ioi
>
>
>>> 3) ArchivedModuleComboTest.java
>>>
>>> 55 Path moduleDir = Files.createTempDirectory(userDir, "mods");
>>>
>>> I don't see anything got placed under the "mods" dir, is it by design?
>> Yes.
>>
>
>
> On 7/6/18 12:34 PM, Jiangli Zhou wrote:
>> Hi Calvin,
>>
>> Thanks for the review! Here is the updated webrevs that address the
>> feedbacks from you and Ioi:
>>
>> http://cr.openjdk.java.net/~jiangli/8202035/webrev_inc.01/
>>
>> Full webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev_full.01/
>>
>>> On Jul 6, 2018, at 9:15 AM, Calvin Cheung wrote:
>>>
>>> Hi Jiangli,
>>>
>>> Thanks for this start-up improvement. The changes look good overall. I've
>>> the following minor comments.
>>>
>>> 1) make/hotspot/symbols/symbols-unix
>>>
>>>134 JVM_InitializeFromArchive
>>>
>>>If you want the symbols to be in alphabetical order, the above should be
>>> moved after JVM_InitStackTraceElementArray.
>> Fixed.
>>
>>> 2) metaspaceShared.cpp
>>>
>>>1927 oop MetaspaceShared::materialize_archived_object(oop obj) {
>>>1928 if (obj != NULL) {
>>>1929 return
>>> G1CollectedHeap::heap()->materialize_archived_object(obj);
>>>1930 }
>>>1931 return NULL;
>>>1932 }
>>>
>>>Instead of two return statements, how about replacing lines 1928 - 1931
>>> with the following?
>>>
>>>return (obj != NULL) ?
>>> G1CollectedHeap::heap()->materialize_archived_object(obj) : NULL;
>> The original format probably is slightly easier to read, so I left it
>> unchanged. Hope that’s okay with you.
>>
>>> 3) ArchivedModuleComboTest.java
>>>
>>> 55 Path moduleDir = Files.createTempDirectory(userDir, "mods");
>>>
>>> I don't see anything got placed under the "mods" dir, is it by design?
>> Yes.
>>
>>> For the "dump with --module-path" cases, there seems to be a missing test
>>> case with "--show-module-resolution" (similar to Test case 2).
>> When --module-path is specified at dump time, system module graph is not
>> archived currently. There is no need for additional test case with
>> --show-module-resolution in this case since all module objects are created
>> as normal.
>>
>>>
>>> 4) CheckArchivedModuleApp.java
>>>
>>> 53 if (expectArchived && wb.isShared(md)) {
>>> 54 System.out.println(name + " is archived. Expected.");
>>> 55 } else if (!expectArchived && !wb.isShared(md)) {
>>> 56 System.out.println(name + " is not archived.
>>> Expected.");
>>> 57 } else if (expectArchived) {
>>> 58 throw new RuntimeException(
>>> 59 "FAILED. " + name + " is not archived. Expect
>>> archived.");
>>> 60 } else {
>>> 61 throw new RuntimeException(
>>> 62 "FAILED. " + name + " is archived. Expect not
>>> archived.");
>>> 63 }
>>>
>>> I'd suggest the following so that the code is easier to understand:
>>>
>>> if (expectArchived) {
>>> if (wb.isShared(md)) {
>>> System.out.println(name + " is archived. Expected.");
>>> } else {
>>> throw new RuntimeException(
>>> "FAILED. " + name + " is not archived. Expect archived.");
>>> }
>>> } else {
>>> if (!wb.isShared(md)) {
>>> System.out.println(name + " is not archived. Expected.");
>>> } else {
>>> throw new RuntimeException(
>>> "FAILED. " + name + " is archived. Expect not archived.");
>>> }
>>> }
>> Reformatted as suggested.
>>
>>> 5) ArchivedModuleWithCustomImageTest.java
>>>
>>> 178 private static void printCommand(String opts[]) {
>>> 179 StringBuilder cmdLine = new StringBuilder();
>>> 180 for (String cmd : opts)
>>> 181 cmdLine.append(cmd).append(' ');
>>> 182 System.out.println("Command line: [" + cmdLine.toString() +
>>> "]");
>>> 183 }
>>>
>>> Consider putting the above method in ProcessTools.java so that
>>> ProcessTools.createJavaProcessBuilder() and the above test can call it and
>>> avoiding duplicate code.
>>> A separate follow-up bug to address this is fine.
>> That sounds good to me. We might need some reformatting for consolidation. I
>> will file a follow-up RFE.
>>
>>> 6) PrintSystemModulesApp.java
>>>
>>> I don't think it is being used?
>> It’s used by ArchivedModuleCompareTest.java. Looks like it was missing from
>> the earlier webrev. Thanks for catching that. The file is included in the
>> updated webrev.
>>
>> Thanks!
>> Jiangli
>>
>>> thanks,
>>> Calvin
>>>
>>> On 6/28/18, 4:15 PM, Jiangli Zhou
Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
Hi Jiangli, On 6/28/18 4:15 PM, Jiangli Zhou wrote:> webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/ RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921 Good work. I'm glad to see a pretty good startup improvement. I reviewed java.base change that looks good. Mandy
Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
Hi Jiangli,
The VM changes look good to me.
For the tests: I think we need a comment here saying that "mods" is
intentionally empty, and also an explanation why it's not necessary to
actually fill with actual modules?
Thanks
- Ioi
3) ArchivedModuleComboTest.java
55 Path moduleDir = Files.createTempDirectory(userDir, "mods");
I don't see anything got placed under the "mods" dir, is it by design?
Yes.
On 7/6/18 12:34 PM, Jiangli Zhou wrote:
Hi Calvin,
Thanks for the review! Here is the updated webrevs that address the feedbacks
from you and Ioi:
http://cr.openjdk.java.net/~jiangli/8202035/webrev_inc.01/
Full webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev_full.01/
On Jul 6, 2018, at 9:15 AM, Calvin Cheung wrote:
Hi Jiangli,
Thanks for this start-up improvement. The changes look good overall. I've the
following minor comments.
1) make/hotspot/symbols/symbols-unix
134 JVM_InitializeFromArchive
If you want the symbols to be in alphabetical order, the above should be
moved after JVM_InitStackTraceElementArray.
Fixed.
2) metaspaceShared.cpp
1927 oop MetaspaceShared::materialize_archived_object(oop obj) {
1928 if (obj != NULL) {
1929 return G1CollectedHeap::heap()->materialize_archived_object(obj);
1930 }
1931 return NULL;
1932 }
Instead of two return statements, how about replacing lines 1928 - 1931
with the following?
return (obj != NULL) ?
G1CollectedHeap::heap()->materialize_archived_object(obj) : NULL;
The original format probably is slightly easier to read, so I left it
unchanged. Hope that’s okay with you.
3) ArchivedModuleComboTest.java
55 Path moduleDir = Files.createTempDirectory(userDir, "mods");
I don't see anything got placed under the "mods" dir, is it by design?
Yes.
For the "dump with --module-path" cases, there seems to be a missing test case with
"--show-module-resolution" (similar to Test case 2).
When --module-path is specified at dump time, system module graph is not
archived currently. There is no need for additional test case with
--show-module-resolution in this case since all module objects are created as
normal.
4) CheckArchivedModuleApp.java
53 if (expectArchived && wb.isShared(md)) {
54 System.out.println(name + " is archived. Expected.");
55 } else if (!expectArchived && !wb.isShared(md)) {
56 System.out.println(name + " is not archived. Expected.");
57 } else if (expectArchived) {
58 throw new RuntimeException(
59 "FAILED. " + name + " is not archived. Expect
archived.");
60 } else {
61 throw new RuntimeException(
62 "FAILED. " + name + " is archived. Expect not
archived.");
63 }
I'd suggest the following so that the code is easier to understand:
if (expectArchived) {
if (wb.isShared(md)) {
System.out.println(name + " is archived. Expected.");
} else {
throw new RuntimeException(
"FAILED. " + name + " is not archived. Expect archived.");
}
} else {
if (!wb.isShared(md)) {
System.out.println(name + " is not archived. Expected.");
} else {
throw new RuntimeException(
"FAILED. " + name + " is archived. Expect not archived.");
}
}
Reformatted as suggested.
5) ArchivedModuleWithCustomImageTest.java
178 private static void printCommand(String opts[]) {
179 StringBuilder cmdLine = new StringBuilder();
180 for (String cmd : opts)
181 cmdLine.append(cmd).append(' ');
182 System.out.println("Command line: [" + cmdLine.toString() + "]");
183 }
Consider putting the above method in ProcessTools.java so that
ProcessTools.createJavaProcessBuilder() and the above test can call it and
avoiding duplicate code.
A separate follow-up bug to address this is fine.
That sounds good to me. We might need some reformatting for consolidation. I
will file a follow-up RFE.
6) PrintSystemModulesApp.java
I don't think it is being used?
It’s used by ArchivedModuleCompareTest.java. Looks like it was missing from the
earlier webrev. Thanks for catching that. The file is included in the updated
webrev.
Thanks!
Jiangli
thanks,
Calvin
On 6/28/18, 4:15 PM, Jiangli Zhou wrote:
This is a follow-up RFE of JDK-8201650 (Move iteration order randomization of
unmodifiable Set and Map to iterators), which was resolved to allow Set/Map
objects being archived at CDS dump time (thanks Claes and Stuart Marks). In the
current RFE, it archives the set of system ModuleReference and ModuleDescriptor
objects (including their referenced objects) in 'open' archive heap region at
CDS dump time. It allows reusing of the objects and bypassing the process of
creating the system ModuleDescriptors and ModuleReferences at
Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
Hi Calvin,
Thanks for the review! Here is the updated webrevs that address the feedbacks
from you and Ioi:
http://cr.openjdk.java.net/~jiangli/8202035/webrev_inc.01/
Full webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev_full.01/
> On Jul 6, 2018, at 9:15 AM, Calvin Cheung wrote:
>
> Hi Jiangli,
>
> Thanks for this start-up improvement. The changes look good overall. I've the
> following minor comments.
>
> 1) make/hotspot/symbols/symbols-unix
>
>134 JVM_InitializeFromArchive
>
>If you want the symbols to be in alphabetical order, the above should be
> moved after JVM_InitStackTraceElementArray.
Fixed.
>
> 2) metaspaceShared.cpp
>
>1927 oop MetaspaceShared::materialize_archived_object(oop obj) {
>1928 if (obj != NULL) {
>1929 return G1CollectedHeap::heap()->materialize_archived_object(obj);
>1930 }
>1931 return NULL;
>1932 }
>
>Instead of two return statements, how about replacing lines 1928 - 1931
> with the following?
>
>return (obj != NULL) ?
> G1CollectedHeap::heap()->materialize_archived_object(obj) : NULL;
The original format probably is slightly easier to read, so I left it
unchanged. Hope that’s okay with you.
>
> 3) ArchivedModuleComboTest.java
>
> 55 Path moduleDir = Files.createTempDirectory(userDir, "mods");
>
> I don't see anything got placed under the "mods" dir, is it by design?
Yes.
>
> For the "dump with --module-path" cases, there seems to be a missing test
> case with "--show-module-resolution" (similar to Test case 2).
When --module-path is specified at dump time, system module graph is not
archived currently. There is no need for additional test case with
--show-module-resolution in this case since all module objects are created as
normal.
>
>
> 4) CheckArchivedModuleApp.java
>
> 53 if (expectArchived && wb.isShared(md)) {
> 54 System.out.println(name + " is archived. Expected.");
> 55 } else if (!expectArchived && !wb.isShared(md)) {
> 56 System.out.println(name + " is not archived. Expected.");
> 57 } else if (expectArchived) {
> 58 throw new RuntimeException(
> 59 "FAILED. " + name + " is not archived. Expect
> archived.");
> 60 } else {
> 61 throw new RuntimeException(
> 62 "FAILED. " + name + " is archived. Expect not
> archived.");
> 63 }
>
> I'd suggest the following so that the code is easier to understand:
>
> if (expectArchived) {
> if (wb.isShared(md)) {
> System.out.println(name + " is archived. Expected.");
> } else {
> throw new RuntimeException(
> "FAILED. " + name + " is not archived. Expect archived.");
> }
> } else {
> if (!wb.isShared(md)) {
> System.out.println(name + " is not archived. Expected.");
> } else {
> throw new RuntimeException(
> "FAILED. " + name + " is archived. Expect not archived.");
> }
> }
Reformatted as suggested.
>
> 5) ArchivedModuleWithCustomImageTest.java
>
> 178 private static void printCommand(String opts[]) {
> 179 StringBuilder cmdLine = new StringBuilder();
> 180 for (String cmd : opts)
> 181 cmdLine.append(cmd).append(' ');
> 182 System.out.println("Command line: [" + cmdLine.toString() + "]");
> 183 }
>
> Consider putting the above method in ProcessTools.java so that
> ProcessTools.createJavaProcessBuilder() and the above test can call it and
> avoiding duplicate code.
> A separate follow-up bug to address this is fine.
That sounds good to me. We might need some reformatting for consolidation. I
will file a follow-up RFE.
>
> 6) PrintSystemModulesApp.java
>
> I don't think it is being used?
It’s used by ArchivedModuleCompareTest.java. Looks like it was missing from the
earlier webrev. Thanks for catching that. The file is included in the updated
webrev.
Thanks!
Jiangli
>
> thanks,
> Calvin
>
> On 6/28/18, 4:15 PM, Jiangli Zhou wrote:
>> This is a follow-up RFE of JDK-8201650 (Move iteration order randomization
>> of unmodifiable Set and Map to iterators), which was resolved to allow
>> Set/Map objects being archived at CDS dump time (thanks Claes and Stuart
>> Marks). In the current RFE, it archives the set of system ModuleReference
>> and ModuleDescriptor objects (including their referenced objects) in 'open'
>> archive heap region at CDS dump time. It allows reusing of the objects and
>> bypassing the process of creating the system ModuleDescriptors and
>> ModuleReferences at runtime for startup improvement. My preliminary
>> measurements on linux-x64 showed ~5% startup improvement when running
>> HelloWorld from -cp using archived module objects at runtime (without extra
>> tuning).
>>
>> The library changes in the following webrev are contributed by Alan Bateman.
>>
Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
Hi Jiangli,
Thanks for this start-up improvement. The changes look good overall.
I've the following minor comments.
1) make/hotspot/symbols/symbols-unix
134 JVM_InitializeFromArchive
If you want the symbols to be in alphabetical order, the above
should be moved after JVM_InitStackTraceElementArray.
2) metaspaceShared.cpp
1927 oop MetaspaceShared::materialize_archived_object(oop obj) {
1928 if (obj != NULL) {
1929 return
G1CollectedHeap::heap()->materialize_archived_object(obj);
1930 }
1931 return NULL;
1932 }
Instead of two return statements, how about replacing lines 1928 -
1931 with the following?
return (obj != NULL) ?
G1CollectedHeap::heap()->materialize_archived_object(obj) : NULL;
3) ArchivedModuleComboTest.java
55 Path moduleDir = Files.createTempDirectory(userDir, "mods");
I don't see anything got placed under the "mods" dir, is it by design?
For the "dump with --module-path" cases, there seems to be a missing
test case with "--show-module-resolution" (similar to Test case 2).
4) CheckArchivedModuleApp.java
53 if (expectArchived && wb.isShared(md)) {
54 System.out.println(name + " is archived. Expected.");
55 } else if (!expectArchived && !wb.isShared(md)) {
56 System.out.println(name + " is not archived.
Expected.");
57 } else if (expectArchived) {
58 throw new RuntimeException(
59 "FAILED. " + name + " is not archived.
Expect archived.");
60 } else {
61 throw new RuntimeException(
62 "FAILED. " + name + " is archived. Expect
not archived.");
63 }
I'd suggest the following so that the code is easier to understand:
if (expectArchived) {
if (wb.isShared(md)) {
System.out.println(name + " is archived. Expected.");
} else {
throw new RuntimeException(
"FAILED. " + name + " is not archived. Expect archived.");
}
} else {
if (!wb.isShared(md)) {
System.out.println(name + " is not archived. Expected.");
} else {
throw new RuntimeException(
"FAILED. " + name + " is archived. Expect not archived.");
}
}
5) ArchivedModuleWithCustomImageTest.java
178 private static void printCommand(String opts[]) {
179 StringBuilder cmdLine = new StringBuilder();
180 for (String cmd : opts)
181 cmdLine.append(cmd).append(' ');
182 System.out.println("Command line: [" + cmdLine.toString()
+ "]");
183 }
Consider putting the above method in ProcessTools.java so that
ProcessTools.createJavaProcessBuilder() and the above test can call it
and avoiding duplicate code.
A separate follow-up bug to address this is fine.
6) PrintSystemModulesApp.java
I don't think it is being used?
thanks,
Calvin
On 6/28/18, 4:15 PM, Jiangli Zhou wrote:
This is a follow-up RFE of JDK-8201650 (Move iteration order randomization of
unmodifiable Set and Map to iterators), which was resolved to allow Set/Map
objects being archived at CDS dump time (thanks Claes and Stuart Marks). In the
current RFE, it archives the set of system ModuleReference and ModuleDescriptor
objects (including their referenced objects) in 'open' archive heap region at
CDS dump time. It allows reusing of the objects and bypassing the process of
creating the system ModuleDescriptors and ModuleReferences at runtime for
startup improvement. My preliminary measurements on linux-x64 showed ~5%
startup improvement when running HelloWorld from -cp using archived module
objects at runtime (without extra tuning).
The library changes in the following webrev are contributed by Alan Bateman.
Thanks Alan and Mandy for discussions and help. Thanks Karen, Lois and Ioi for
discussion and suggestions on initialization ordering.
The majority of the module object archiving code are in heapShared.hpp and
heapShared.cpp. Thanks Coleen for pre-review and Eric Caspole for helping
performance tests.
webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/
RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921
Tested using tier1 - tier6 via mach5 including all new test cases added in the
webrev.
Following are the details of system module archiving, which are duplicated in
above bug report.
---
Support archiving system module graph when the initial module is unnamed module
from -cp currently.
Support G1 GC, 64-bit (non-Windows). Requires UseCompressedOops and
UseCompressedClassPointers.
Dump time system module object archiving
=
At dump time, the following fields in ArchivedModuleGraph are set to record the
system module information created by
Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
Hi Ioi,
Thanks for the review!
> On Jul 5, 2018, at 5:45 PM, Ioi Lam wrote:
>
> Hi Jiangli,
>
> Thank you so much for working on this. I think it's great that we can get the
> start-up improvement by archiving the ModuleDescriptor.
>
> I just have some coding style comments regarding heapShared.cpp. This file
> contains the code for coping objects and relocating pointers. By its nature,
> this kind of code is usually complicated, so I think we should try to make
> it as easy to understand as possible.
>
>
> [1] HeapShared::walk_from_field_and_archiving:
>
> This name is not grammatically correct. How about
> HeapShared::archive_reachable_objects_from_static_field
Sounds good.
>
> [2] How about changing the parameter field_offset -> static_field_offset
> When I first read the code I was confused whether it's talking
> about static or instance fields. Usually, "field"
> implies instance field, so it's better to specifically
> say "static field”.
Ok.
>
> [3] This code would fail if "f" is already archived.
>
> 473 // get the archived copy of the field referenced object
> 474 oop af = MetaspaceShared::archive_heap_object(f, THREAD);
> 475 WalkOopAndArchiveClosure walker(1, subgraph_info, f, af);
> 476 f->oop_iterate();
Hmmm, it’s possible we might encounter an archived object during reference
walking & archiving in future cases. I’ll add a check.
>
> [4] There's duplicated code between walk_from_field_and_archiving and
> WalkOopAndArchiveClosure::do_oop_work
>
> 403 assert(relocated_k == MetaspaceShared::get_relocated_klass(orig_k),
> 404 "must be the relocated Klass in the shared space");
> 405 _subgraph_info->add_subgraph_object_klass(orig_k, relocated_k);
>
> - vs -
>
> 484 assert(relocated_k == MetaspaceShared::get_relocated_klass(orig_k),
> 485 "must be the relocated Klass in the shared space");
> 486 subgraph_info->add_subgraph_object_klass(orig_k, relocated_k);
I’ll move the assert into add_subgraph_object_klass().
>
> [5] This code is also duplicated:
>
> 375 RawAccess::oop_store(new_p, archived);
> 376 log.print("--- archived copy existing, store archived " PTR_FORMAT
> " in " PTR_FORMAT,
> 377 p2i(archived), p2i(new_p));
>
> - vs -
>
> 395 RawAccess::oop_store(new_p, archived);
> 396 log.print("=== store archived " PTR_FORMAT " in " PTR_FORMAT,
> 397p2i(archived), p2i(new_p));
The first case is for existing archived copy and the second is for newly
archived. The different logging messages are helpful for debugging. Not sure if
using a function to encapsulate the store & log worth it in this case. Any
suggestion?
>
> [6] This code, even though it's correct, is hard to understand --
> why are we calculating the distance between the two objects?
>
> 368 size_t delta = pointer_delta((HeapWord*)_archived_referencing_obj,
> 369 (HeapWord*)_orig_referencing_obj);
> 370 T* new_p = (T*)((HeapWord*)p + delta);
>
> I thin it would be easier to understand if we change the order of the
> two arithmetic operations:
>
> // new_p is the address of the same field inside
> _archived_referencing_obj.
> size_t field_offset_in_bytes = pointer_delta(p, _orig_referencing_obj, 1);
> T* new_p = (T*)(address(_orig_referencing_obj) + field_offset_in_bytes);
I think this works too. I’ll change as you suggested.
>
> [7] I have a hard time understand this log:
>
> 376 log.print("--- archived copy existing, store archived " PTR_FORMAT
> " in " PTR_FORMAT,
> 377 p2i(archived), p2i(new_p));
>
> How about this?
>
> log.print("--- updated embedded pointer @[" PTR_FORMAT "] => " PTR_FORMAT,
> p2i(new_p), p2i(archived));
It is for the case where there is an existing copy of the archived object.
Maybe ‘found existing archived copy’ would help?
>
>
> For your consideration, I've incorporated my comments above into
> heapShared.cpp.
> I've not tested it so it most likely won't build :-(
>
>
> http://cr.openjdk.java.net/~iklam/misc/heapShared.old.cpp [your version]
> http://cr.openjdk.java.net/~iklam/misc/heapShared.new.cpp [my version]
>
> Please take a look and see if you like it.
Thanks a lot! I’ll take a look and incorporate your suggestions.
Thanks again!
Jiangli
>
> Thanks
> - Ioi
>
> On 6/28/18 4:15 PM, Jiangli Zhou wrote:
>> This is a follow-up RFE of JDK-8201650 (Move iteration order randomization
>> of unmodifiable Set and Map to iterators), which was resolved to allow
>> Set/Map objects being archived at CDS dump time (thanks Claes and Stuart
>> Marks). In the current RFE, it archives the set of system ModuleReference
>> and ModuleDescriptor objects (including their referenced objects) in 'open'
>> archive heap region at CDS dump time. It allows reusing of the objects and
>> bypassing the process of creating the system
Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
Hi Jiangli,
Thank you so much for working on this. I think it's great that we can
get the
start-up improvement by archiving the ModuleDescriptor.
I just have some coding style comments regarding heapShared.cpp. This file
contains the code for coping objects and relocating pointers. By its nature,
this kind of code is usually complicated, so I think we should try to make
it as easy to understand as possible.
[1] HeapShared::walk_from_field_and_archiving:
This name is not grammatically correct. How about
HeapShared::archive_reachable_objects_from_static_field
[2] How about changing the parameter field_offset -> static_field_offset
When I first read the code I was confused whether it's talking
about static or instance fields. Usually, "field"
implies instance field, so it's better to specifically
say "static field".
[3] This code would fail if "f" is already archived.
473 // get the archived copy of the field referenced object
474 oop af = MetaspaceShared::archive_heap_object(f, THREAD);
475 WalkOopAndArchiveClosure walker(1, subgraph_info, f, af);
476 f->oop_iterate();
[4] There's duplicated code between walk_from_field_and_archiving and
WalkOopAndArchiveClosure::do_oop_work
403 assert(relocated_k ==
MetaspaceShared::get_relocated_klass(orig_k),
404 "must be the relocated Klass in the shared space");
405 _subgraph_info->add_subgraph_object_klass(orig_k, relocated_k);
- vs -
484 assert(relocated_k ==
MetaspaceShared::get_relocated_klass(orig_k),
485 "must be the relocated Klass in the shared space");
486 subgraph_info->add_subgraph_object_klass(orig_k, relocated_k);
[5] This code is also duplicated:
375 RawAccess::oop_store(new_p, archived);
376 log.print("--- archived copy existing, store archived "
PTR_FORMAT " in " PTR_FORMAT,
377 p2i(archived), p2i(new_p));
- vs -
395 RawAccess::oop_store(new_p, archived);
396 log.print("=== store archived " PTR_FORMAT " in " PTR_FORMAT,
397 p2i(archived), p2i(new_p));
[6] This code, even though it's correct, is hard to understand --
why are we calculating the distance between the two objects?
368 size_t delta = pointer_delta((HeapWord*)_archived_referencing_obj,
369 (HeapWord*)_orig_referencing_obj);
370 T* new_p = (T*)((HeapWord*)p + delta);
I thin it would be easier to understand if we change the order of the
two arithmetic operations:
// new_p is the address of the same field inside
_archived_referencing_obj.
size_t field_offset_in_bytes = pointer_delta(p,
_orig_referencing_obj, 1);
T* new_p = (T*)(address(_orig_referencing_obj) +
field_offset_in_bytes);
[7] I have a hard time understand this log:
376 log.print("--- archived copy existing, store archived "
PTR_FORMAT " in " PTR_FORMAT,
377 p2i(archived), p2i(new_p));
How about this?
log.print("--- updated embedded pointer @[" PTR_FORMAT "] => "
PTR_FORMAT,
p2i(new_p), p2i(archived));
For your consideration, I've incorporated my comments above into
heapShared.cpp.
I've not tested it so it most likely won't build :-(
http://cr.openjdk.java.net/~iklam/misc/heapShared.old.cpp [your version]
http://cr.openjdk.java.net/~iklam/misc/heapShared.new.cpp [my version]
Please take a look and see if you like it.
Thanks
- Ioi
On 6/28/18 4:15 PM, Jiangli Zhou wrote:
This is a follow-up RFE of JDK-8201650 (Move iteration order randomization of
unmodifiable Set and Map to iterators), which was resolved to allow Set/Map
objects being archived at CDS dump time (thanks Claes and Stuart Marks). In the
current RFE, it archives the set of system ModuleReference and ModuleDescriptor
objects (including their referenced objects) in 'open' archive heap region at
CDS dump time. It allows reusing of the objects and bypassing the process of
creating the system ModuleDescriptors and ModuleReferences at runtime for
startup improvement. My preliminary measurements on linux-x64 showed ~5%
startup improvement when running HelloWorld from -cp using archived module
objects at runtime (without extra tuning).
The library changes in the following webrev are contributed by Alan Bateman.
Thanks Alan and Mandy for discussions and help. Thanks Karen, Lois and Ioi for
discussion and suggestions on initialization ordering.
The majority of the module object archiving code are in heapShared.hpp and
heapShared.cpp. Thanks Coleen for pre-review and Eric Caspole for helping
performance tests.
webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/
RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921
Tested using tier1 - tier6 via mach5 including all new test cases added in the
webrev.
Following are the details of system module archiving, which are duplicated in
above bug report.
Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
Hi Erik, Thank you for the quick review! Jiangli > On Jun 28, 2018, at 5:44 PM, Erik Joelsson wrote: > > Build changes look good. > > /Erik > > > On 2018-06-28 16:15, Jiangli Zhou wrote: >> This is a follow-up RFE of JDK-8201650 (Move iteration order randomization >> of unmodifiable Set and Map to iterators), which was resolved to allow >> Set/Map objects being archived at CDS dump time (thanks Claes and Stuart >> Marks). In the current RFE, it archives the set of system ModuleReference >> and ModuleDescriptor objects (including their referenced objects) in 'open' >> archive heap region at CDS dump time. It allows reusing of the objects and >> bypassing the process of creating the system ModuleDescriptors and >> ModuleReferences at runtime for startup improvement. My preliminary >> measurements on linux-x64 showed ~5% startup improvement when running >> HelloWorld from -cp using archived module objects at runtime (without extra >> tuning). >> >> The library changes in the following webrev are contributed by Alan Bateman. >> Thanks Alan and Mandy for discussions and help. Thanks Karen, Lois and Ioi >> for discussion and suggestions on initialization ordering. >> >> The majority of the module object archiving code are in heapShared.hpp and >> heapShared.cpp. Thanks Coleen for pre-review and Eric Caspole for helping >> performance tests. >> >> webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/ >> RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921 >> >> Tested using tier1 - tier6 via mach5 including all new test cases added in >> the webrev. >> >> Following are the details of system module archiving, which are duplicated >> in above bug report. >> --- >> Support archiving system module graph when the initial module is unnamed >> module from -cp currently. >> >> Support G1 GC, 64-bit (non-Windows). Requires UseCompressedOops and >> UseCompressedClassPointers. >> >> Dump time system module object archiving >> = >> At dump time, the following fields in ArchivedModuleGraph are set to record >> the system module information created by ModuleBootstrap for archiving. >> >> private static SystemModules archivedSystemModules; >> private static ModuleFinder archivedSystemModuleFinder; >> private static String archivedMainModule; >> >> The archiving process starts from a given static field in >> ArchivedModuleGraph class instance (java mirror object). The process >> archives the complete network of java heap objects that are reachable >> directly or indirectly from the starting object by following references. >> >> 1. Starts from a given static field within the Class instance (java mirror). >> If the static field is a refererence field and points to a non-null java >> object, proceed to the next step. The static field and it's value is >> recorded and stored outside the archived mirror. >> 2. Archives the referenced java object. If an archived copy of the current >> object already exists, updates the pointer in the archived copy of the >> referencing object to point to the current archived object. Otherwise, >> proceed to the next step. >> 3. Follows all references within the current java object and recursively >> archive the sub-graph of objects starting from each reference encountered >> within the object. >> 4. Updates the pointer in the archived copy of referecing object to point to >> the current archived object. >> 5. The Klass of the current java object is added to a list of Klasses for >> loading and initializing before any object in the archived graph can be >> accessed at runtime. >> >> Runtime initialization from archived system module objects >> >> VM.initializeFromArchive() is called from ArchivedModuleGraph's >> static initializer to initialize from the archived module information. >> Klasses in the recorded list are loaded, linked and initialized. The static >> fields in ArchivedModuleGraph class instance are initialized using the >> archived field values. After initialization, the archived system module >> objects can be used directly. >> >> If the archived java heap data is not successfully mapped at runtime, or >> there is an error during VM.initializeFromArchive(), then all static fields >> in ArchivedModuleGraph are not initialized. In that case, system >> ModuleDescriptor and ModuleReference objects are created as normal. >> >> In non-CDS mode, VM.initializeFromArchive() returns immediately with >> minimum added overhead for normal execution. >> >> Thanks, >> Jiangli >> >> >
Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules
Build changes look good. /Erik On 2018-06-28 16:15, Jiangli Zhou wrote: This is a follow-up RFE of JDK-8201650 (Move iteration order randomization of unmodifiable Set and Map to iterators), which was resolved to allow Set/Map objects being archived at CDS dump time (thanks Claes and Stuart Marks). In the current RFE, it archives the set of system ModuleReference and ModuleDescriptor objects (including their referenced objects) in 'open' archive heap region at CDS dump time. It allows reusing of the objects and bypassing the process of creating the system ModuleDescriptors and ModuleReferences at runtime for startup improvement. My preliminary measurements on linux-x64 showed ~5% startup improvement when running HelloWorld from -cp using archived module objects at runtime (without extra tuning). The library changes in the following webrev are contributed by Alan Bateman. Thanks Alan and Mandy for discussions and help. Thanks Karen, Lois and Ioi for discussion and suggestions on initialization ordering. The majority of the module object archiving code are in heapShared.hpp and heapShared.cpp. Thanks Coleen for pre-review and Eric Caspole for helping performance tests. webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/ RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921 Tested using tier1 - tier6 via mach5 including all new test cases added in the webrev. Following are the details of system module archiving, which are duplicated in above bug report. --- Support archiving system module graph when the initial module is unnamed module from -cp currently. Support G1 GC, 64-bit (non-Windows). Requires UseCompressedOops and UseCompressedClassPointers. Dump time system module object archiving = At dump time, the following fields in ArchivedModuleGraph are set to record the system module information created by ModuleBootstrap for archiving. private static SystemModules archivedSystemModules; private static ModuleFinder archivedSystemModuleFinder; private static String archivedMainModule; The archiving process starts from a given static field in ArchivedModuleGraph class instance (java mirror object). The process archives the complete network of java heap objects that are reachable directly or indirectly from the starting object by following references. 1. Starts from a given static field within the Class instance (java mirror). If the static field is a refererence field and points to a non-null java object, proceed to the next step. The static field and it's value is recorded and stored outside the archived mirror. 2. Archives the referenced java object. If an archived copy of the current object already exists, updates the pointer in the archived copy of the referencing object to point to the current archived object. Otherwise, proceed to the next step. 3. Follows all references within the current java object and recursively archive the sub-graph of objects starting from each reference encountered within the object. 4. Updates the pointer in the archived copy of referecing object to point to the current archived object. 5. The Klass of the current java object is added to a list of Klasses for loading and initializing before any object in the archived graph can be accessed at runtime. Runtime initialization from archived system module objects VM.initializeFromArchive() is called from ArchivedModuleGraph's static initializer to initialize from the archived module information. Klasses in the recorded list are loaded, linked and initialized. The static fields in ArchivedModuleGraph class instance are initialized using the archived field values. After initialization, the archived system module objects can be used directly. If the archived java heap data is not successfully mapped at runtime, or there is an error during VM.initializeFromArchive(), then all static fields in ArchivedModuleGraph are not initialized. In that case, system ModuleDescriptor and ModuleReference objects are created as normal. In non-CDS mode, VM.initializeFromArchive() returns immediately with minimum added overhead for normal execution. Thanks, Jiangli
