<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Ioi,<div class=""><br class=""></div><div class="">Thanks for getting back to me.</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 7, 2017, at 5:45 PM, Ioi Lam <<a href="mailto:ioi.lam@oracle.com" class="">ioi.lam@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" class="">
<div text="#000000" bgcolor="#FFFFFF" class=""><p class="">On 8/4/17 10:19 PM, Jiangli Zhou wrote:<br class="">
</p>
<blockquote type="cite" cite="mid:AFA96554-9805-4758-A074-8F7E870696AC@oracle.com" class="">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" class="">
Hi Ioi,
<div class=""><br class="">
</div>
<div class="">Thanks for looking again.</div>
<div class=""><br class="">
<div class="">
<blockquote type="cite" class="">
<div class="">On Aug 4, 2017, at 2:22 PM, Ioi Lam <<a href="mailto:ioi.lam@oracle.com" class="" moz-do-not-send="true">ioi.lam@oracle.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class=""> <tt class="">Hi Jiangli,</tt><tt class=""><br class="">
</tt><tt class=""><br class="">
The code looks good in general. I just have a few pet
peeves for readability:<br class="">
<br class="">
<br class="">
</tt><tt class="">(1) stringTable.cpp and
metaspaceShared.cpp have the same asserts</tt><tt class=""><br class="">
</tt><tt class=""><br class="">
</tt><tt class=""> 704 assert(UseG1GC, "Only support
G1 GC");</tt><tt class=""><br class="">
</tt><tt class=""> 705 assert(UseCompressedOops
&& UseCompressedClassPointers,</tt><tt class=""><br class="">
</tt><tt class=""> 706 "Only support
UseCompressedOops and UseCompressedClassPointers
enabled");</tt><tt class=""><br class="">
</tt><tt class=""><br class="">
</tt><tt class="">1615 assert(UseG1GC, "Only support
G1 GC");</tt><tt class=""><br class="">
</tt><tt class="">1616 assert(UseCompressedOops
&& UseCompressedClassPointers,</tt><tt class=""><br class="">
</tt><tt class="">1617 "Only support
UseCompressedOops and UseCompressedClassPointers
enabled");</tt><tt class=""><br class="">
</tt><tt class=""><br class="">
</tt><tt class="">Maybe it's better to combine them into
a single function like
MetaspaceShared::assert_vm_flags() so they don't get
out of sync?</tt><tt class=""><br class="">
</tt></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
There is a MetaspaceShared::allow_archive_heap_object(), which
checks for UseG1GC, UseCompressedOops and
UseCompressedClassPointers combined. It does not seem to worth
add another separate API for asserting the required flags.
I’ll use that in the assert. </div>
<div class=""><br class="">
</div>
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><tt class=""> </tt><tt class=""><br class="">
<br class="">
<br class="">
</tt><tt class="">(2)
FileMapInfo::write_archive_heap_regions()</tt><tt class=""><br class="">
</tt><tt class=""><br class="">
</tt><tt class="">I still find this code very hard to
read, especially due to the loop.<br class="">
<br class="">
First, the comments are not consistent with the code:</tt><tt class=""><br class="">
</tt><tt class=""><br class="">
</tt><tt class=""> 498 assert(arr_len <=
max_num_regions, "number of memory regions exceeds
maximum");</tt><tt class=""><br class="">
<br class="">
</tt><tt class="">but the comments says: "The rest are
consecutive full GC regions" which means there's a
chance for max_num_regions to be more than 2 (which
will be the case with Calvin's java-loader dumping
changes using very small heap size).</tt><tt class="">
So the code is actually wrong.<br class="">
</tt></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">The max_num_regions is the maximum number of region for
each archived heap space (the string space, or open archive
space). We only run into the case where the MemRegion array
size is larger than max_num_regions with Calvin’s pending
change. As part of Calvin’s change, he will change the
assert into a check and bail out if the number of MemRegions
are larger than max_num_regions due to heap fragmentation.</div>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
</div>
</div>
</blockquote>
Your latest patch assumes that arr_len <= 2, but the
implementation of
G1CollectedHeap::heap()->begin_archive_alloc_range() /
G1CollectedHeap::heap()->end_archive_alloc_range() actually
allows more than 2 regions to returned. So simply putting an assert
there seems risky (unless you have analyzed all possible scenarios
to prove that's impossible).<br class="">
<br class="">
Instead of trying to come up with a complicated proof, I think it's
much safer to disable the archived string region if the arr_len >
2. Also, if the string region is disabled, you should also disable
the open_archive_heap_region<br class="">
<br class="">
I think this is a general issue with the mapped heap regions, and it
just happens to be revealed by Calvin's patch. So we should fix it
now and not wait for Calvin's patch.<br class=""></div></div></blockquote><div><br class=""></div><div>Ok. I’ll change the assert to be a check.</div><br class=""><blockquote type="cite" class=""><div class=""><div text="#000000" bgcolor="#FFFFFF" class="">
<br class="">
<br class="">
<blockquote type="cite" cite="mid:AFA96554-9805-4758-A074-8F7E870696AC@oracle.com" class="">
<div class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><tt class=""> </tt><tt class=""><br class="">
</tt><tt class="">The word "region" is used in these
parameters, but they don't mean the same thing.<br class="">
<br class="">
</tt><tt class="">
GrowableArray<MemRegion> *regions</tt><tt class=""><br class="">
</tt><tt class=""> int first_region, int
max_num_regions,</tt><tt class=""><br class="">
</tt><tt class=""><br class="">
<br class="">
How about regions -> g1_regions_list<br class="">
first_region -> first_region_in_archive<br class="">
</tt></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">The GrowableArray above is the MemRegions that GC code
gives back to us. The GC code combines multiple G1 regions.
The comments probably are over-explaining the details, which
are hidden in the GC code. Probably that’s the confusing
source. I’ll make the comment more clear. </div>
<div class=""><br class="">
</div>
<div class="">Using g1_regions_list would also be confusing, since <span style="font-family: Menlo;" class="">write_archive_heap_regions</span> does
not handle G1 regions directly. It processes the MemRegion
array that GC code returns. How about changing ‘regions’ to
‘mem_regions’ or ‘archive_regions'?</div>
<br class="">
</div>
</div>
</blockquote>
How about heap_regions? These are regions in the active Java heap,
which current has not mapped anything from the CDS archive.<br class=""></div></div></blockquote><div><br class=""></div>Ok.</div><div><br class=""></div><div>I’m updating my changes and will send out a consolidated webrev.</div><div><br class=""></div><div>Thanks!</div><div>Jiangli</div><div><br class=""><blockquote type="cite" class=""><div class=""><div text="#000000" bgcolor="#FFFFFF" class="">
<br class="">
<br class="">
<blockquote type="cite" cite="mid:AFA96554-9805-4758-A074-8F7E870696AC@oracle.com" class="">
<div class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><tt class=""> <br class="">
<br class="">
In the comments, I find the phrase 'the current
archive heap region' ambiguous. It could be
(erroneously) interpreted as "a region from the
currently mapped archive”</tt></div>
</div>
</blockquote>
</div>
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><tt class=""><br class="">
To make it unambiguous, how about changing<br class="">
<br class="">
<br class="">
464 // Write the current archive heap region, which
contains one or multiple GC(G1) regions.<br class="">
<br class="">
<br class="">
to<br class="">
<br class="">
// Write the given list of G1 memory regions into
the archive, starting at </tt><br class="">
<tt class=""><tt class=""> // first_region_in_archive</tt>.<br class="">
</tt></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
<div class="">Ok. How about the following:</div>
<div class=""><br class="">
</div>
<div class="">// Write the given list of java heap memory regions into
the archive, starting at </div>
<div class=""><tt class="">// first_region_in_archive</tt>.</div>
<br class="">
</div>
</div>
</blockquote>
Sounds good.<br class="">
<br class="">
Thanks<br class="">
- Ioi<br class="">
<br class="">
<blockquote type="cite" cite="mid:AFA96554-9805-4758-A074-8F7E870696AC@oracle.com" class="">
<div class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><tt class=""> <br class="">
<br class="">
Also, for the explanation of how the G1 regions are
written into the archive, how about:<br class="">
<br class="">
// The G1 regions in the list are sorted in
ascending address order. When there are more objects<br class="">
// than the capacity of a single G1 region, the
bottom-most G1 region may be partially filled, and the<br class="">
// remaining G1 region(s) are consecutively
allocated and fully filled.<br class="">
//<br class="">
// Thus, the bottom-most G1 region (if not empty)
is written into </tt><tt class=""><tt class="">first_region_in_archive</tt>.<br class="">
// The remaining G1 regions (if exist) are
coalesced and written as a single block<br class="">
// into (</tt><tt class=""><tt class=""><tt class="">first_region_in_archive </tt>+ 1)</tt> <br class="">
<br class="">
// Here's the mapping from (g1 regions) ->
(archive regions).<br class="">
<br class="">
<br class="">
</tt><tt class="">All this function needs to do is to
decide the values for </tt><tt class=""><br class="">
</tt><tt class=""><br class="">
</tt><tt class=""> r0_start, r0_top</tt><tt class=""><br class="">
</tt><tt class=""> r1_start, r1_top</tt><tt class=""><br class="">
</tt><tt class=""> </tt><tt class=""><br class="">
</tt><tt class="">I think it would be much better to not
use the loop, and not use the max_num_regions
parameter (it's always 2 anyway).</tt><tt class=""><br class="">
</tt><tt class=""><br class="">
*r0_start = *r0_top = NULL;<br class="">
*r1_start = *r1_top = NULL;<br class="">
<br class="">
</tt><tt class=""> if (arr_len >= 1) {</tt><tt class=""><br class="">
</tt><tt class=""> *r0_start =
regions->at(0).start();</tt><tt class=""><br class="">
</tt><tt class=""> *r0_end = *r0_start +
regions->at(0).byte_size();</tt><tt class=""><br class="">
</tt><tt class=""> }</tt><tt class=""><br class="">
</tt><tt class=""> if (arr_len >= 2) {</tt><tt class=""><br class="">
</tt><tt class=""> int last = arr_len - 1;</tt><tt class=""><br class="">
</tt><tt class=""> *r1_start =
regions->at(1).start();</tt><tt class=""><br class="">
</tt><tt class=""> *r1_end =
regions->at(last).start() +
regions->at(last).byte_size();</tt><tt class=""><br class="">
</tt><tt class=""> }<br class="">
<br class="">
what do you think?<br class="">
</tt></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
<div class="">We need to write out all archive regions including the
empty ones. The loop using max_num_regions is the easiest
way. I’d like to remove the code that deals with r0_* and
r1_ explicitly. Let me try that.</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><tt class=""> <br class="">
<br class="">
<br class="">
(3) metaspace.cpp<br class="">
<br class="">
3350 // Map the archived heap regions after
compressed pointers<br class="">
3351 // because it relies on compressed class
pointers setting to work<br class="">
<br class="">
do you mean this?<br class="">
<br class="">
// Archived heap regions depend on the parameters
of compressed class pointers, so<br class="">
// they must be mapped after such parameters have
been decided in the above call.<br class="">
</tt></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
Hmmm, maybe use ‘arguments’ instead of ‘parameters’?</div>
<div class=""> <br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><tt class=""> <br class="">
<br class="">
(4) I found this name not strictly grammatical. How
about this:<br class="">
<br class="">
allow_archive_heap_object ->
is_heap_object_archiving_allowed<br class="">
</tt></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
Ok.</div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><tt class=""> <br class="">
(5) in most of your code, 'archive' is used as a noun,
except in StringTable::archive_string() where it's
used as a verb.<br class="">
<br class="">
archive_string could also be interpreted erroneously
as "return a string that's already in the archive". So
to be consistent and unambiguous, I think it's better
to rename it to StringTable::create_archived_string()<br class="">
</tt></div>
</div>
</blockquote>
<div class=""><br class="">
</div>
Ok.</div>
<div class=""><br class="">
</div>
<div class="">Thanks,</div>
<div class="">Jiangli</div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><tt class=""> <br class="">
<br class="">
Thanks<br class="">
- Ioi<br class="">
</tt><tt class=""> </tt><tt class=""><br class="">
</tt><br class="">
<div class="moz-cite-prefix">On 8/3/17 5:15 PM, Jiangli
Zhou wrote:<br class="">
</div>
<blockquote cite="mid:98EF8CF9-FD28-46BC-8D3D-52DEA205EBD5@oracle.com" type="cite" class="">Here are the updated webrevs.<br class="">
<br class="">
<a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ejiangli/8179302/webrev.hotspot.02/" class="">http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.02/</a><br class="">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejiangli/8179302/webrev.whitebox.02/" moz-do-not-send="true">http://cr.openjdk.java.net/~jiangli/8179302/webrev.whitebox.02/</a>
<div class=""><br class="">
</div>
<div class="">Changes in the updated webrevs include:</div>
<div class="">
<ul class="MailOutline">
<li class="">Merge with Ioi’s recent shared space
auto-sizing change (<span style="font-family:
'Helvetica Neue';" class="">8072061)</span></li>
<li class=""><span style="font-family: 'Helvetica
Neue';" class="">Addressed all feedbacks from
Ioi and Coleen (Thanks for detailed review!)</span></li>
</ul>
<div class=""><font class="" face="Helvetica Neue"><br class="">
</font></div>
<div class=""><font class="" face="Helvetica Neue">Thanks,</font></div>
<div class=""><font class="" face="Helvetica Neue">Jiangli</font></div>
<br class="">
<br class="">
<blockquote type="cite" class="">On Aug 1, 2017, at
5:29 PM, Jiangli Zhou <a class="moz-txt-link-rfc2396E" href="mailto:jiangli.zhou@Oracle.COM" moz-do-not-send="true"><jiangli.zhou@Oracle.COM></a>
wrote:<br class="">
<br class="">
Hi Ioi,<br class="">
<br class="">
Thank you so much for reviewing this. I’ve
addressed all your feedbacks. Please see details
below. I’ll updated the webrev after addressing
Coleen’s comments.<br class="">
<br class="">
<blockquote type="cite" class="">On Jul 30, 2017,
at 9:07 PM, Ioi Lam <a class="moz-txt-link-rfc2396E" href="mailto:ioi.lam@oracle.com" moz-do-not-send="true"><ioi.lam@oracle.com></a>
wrote:<br class="">
<br class="">
Hi Jiangli,<br class="">
<br class="">
Here are my comments. I've not reviewed the GC
code and I'll leave that to the GC experts :-)<br class="">
<br class="">
stringTable.cpp: StringTable::archive_string<br class="">
<br class="">
add assert for DumpSharedSpaces only<br class="">
</blockquote>
<br class="">
Ok.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
filemap.cpp<br class="">
<br class="">
525 void
FileMapInfo::write_archive_heap_regions(GrowableArray<MemRegion>
*regions,<br class="">
526
int first_region, int num_regions) {<br class="">
<br class="">
When I first read this function, I found it hard
to follow, especially this part that coalesces
the trailing regions:<br class="">
<br class="">
537 int len = regions->length();<br class="">
538 if (len > 1) {<br class="">
539 start =
(char*)regions->at(1).start();<br class="">
540 size = (char*)regions->at(len -
1).end() - start;<br class="">
541 }<br class="">
542 }<br class="">
<br class="">
The rest of filemap.cpp always perform identical
operations on MemRegion arrays, which are either
1 or 2 in size. However, this function doesn't
follow that pattern; it also has a very
different notion of "region", and the confusing
part is regions->size() is not the same as
num_regions.<br class="">
<br class="">
How about we change the API to something like
the following? Before calling this API, the
caller needs to coalesce the trailing G1 regions
into a single MemRegion.<br class="">
<br class="">
FileMapInfo::write_archive_heap_regions(MemRegion
*regions, int first, int num_regions) {<br class="">
if (first ==
MetaspaceShared::first_string) {<br class="">
assert(num_regons <=
MetaspaceShared::max_strings, "...");<br class="">
} else {<br class="">
assert(first ==
MetaspaceShared::first_open_archive_heap_region,
"...");<br class="">
assert(num_regons <=
MetaspaceShared::max_open_archive_heap_region,
"...");<br class="">
}<br class="">
....<br class="">
<br class="">
<br class="">
</blockquote>
<br class="">
I’ve reworked the function and simplified the
code. <br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
756 if (!string_data_mapped) {<br class="">
757
StringTable::ignore_shared_strings(true);<br class="">
758 assert(string_ranges == NULL &&
num_string_ranges == 0, "sanity");<br class="">
759 }<br class="">
760<br class="">
761 if (open_archive_heap_data_mapped) {<br class="">
762
MetaspaceShared::set_open_archive_heap_region_mapped();<br class="">
763 } else {<br class="">
764 assert(open_archive_heap_ranges == NULL
&& num_open_archive_heap_ranges == 0,
"sanity");<br class="">
765 }<br class="">
<br class="">
Maybe the two "if" statements should be more
consistent? Instead of
StringTable::ignore_shared_strings, how
about StringTable::set_shared_strings_region_mapped()?<br class="">
</blockquote>
<br class="">
Fixed.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
FileMapInfo::map_heap_data() --<br class="">
<br class="">
818 char* addr = (char*)regions[i].start();<br class="">
819 char* base = os::map_memory(_fd,
_full_path, si->_file_offset,<br class="">
820 addr,
regions[i].byte_size(), si->_read_only,<br class="">
821
si->_allow_exec);<br class="">
<br class="">
What happens when the first region succeeds to
map but the second region fails to map? Will
both regions be unmapped? I don't see where you
store the return value (base) from
os::map_memory(). Does it mean the code assumes
that (addr == base). If so, we need an assert
here.<br class="">
</blockquote>
<br class="">
If any of the region fails to map, we bail out and
call dealloc_archive_heap_regions(), which handles
the deallocation of any regions specified. If
second region fails to map, all memory ranges
specified by ‘regions’ array are deallocated. We
don’t unmap the memory here since it is part of
the java heap. Unmapping of heap memory are
handled by GC code. The ‘if’ check below makes
sure base == addr.<br class="">
<br class="">
if (base == NULL || base != addr) {<br class="">
// dealloc the regions from java heap<br class="">
dealloc_archive_heap_regions(regions,
region_num);<br class="">
if (log_is_enabled(Info, cds)) {<br class="">
log_info(cds)("UseSharedSpaces: Unable to
map at required address in java heap.");<br class="">
}<br class="">
return false;<br class="">
}<br class="">
<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
constantPool.cpp<br class="">
<br class="">
Handle refs_handle;<br class="">
...<br class="">
refs_handle = Handle(THREAD, (oop)archived);<br class="">
<br class="">
This will first create a NULL handle, then
construct a temporary handle, and then assign
the temp handle back to the null handle. This
means two handles will be pushed onto
THREAD->metadata_handles()<br class="">
<br class="">
I think it's more efficient if you merge these
into a single statement<br class="">
<br class="">
Handle refs_handle(THREAD, (oop)archived);<br class="">
</blockquote>
<br class="">
Fixed.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
Is this experimental code? Maybe it should be
removed?<br class="">
<br class="">
664 if (tag_at(index).is_unresolved_klass())
{<br class="">
665 #if 0<br class="">
666 CPSlot entry = cp->slot_at(index);<br class="">
667 Symbol* name = entry.get_symbol();<br class="">
668 Klass* k =
SystemDictionary::find(name, NULL, NULL,
THREAD);<br class="">
669 if (k != NULL) {<br class="">
670 klass_at_put(index, k);<br class="">
671 }<br class="">
672 #endif<br class="">
673 } else<br class="">
</blockquote>
<br class="">
Removed.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
cpCache.hpp:<br class="">
<br class="">
u8 _archived_references<br class="">
<br class="">
shouldn't this be declared as an narrowOop to
avoid the type casts when it's used?<br class="">
</blockquote>
<br class="">
Ok. <br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
cpCache.cpp:<br class="">
<br class="">
add assert so that one of these is used only
at dump time and the other only at run time?<br class="">
<br class="">
610 oop ConstantPoolCache::archived_references()
{<br class="">
611 return
oopDesc::decode_heap_oop((narrowOop)_archived_references);<br class="">
612 }<br class="">
613<br class="">
614 void
ConstantPoolCache::set_archived_references(oop
o) {<br class="">
615 _archived_references =
(u8)oopDesc::encode_heap_oop(o);<br class="">
616 }<br class="">
</blockquote>
<br class="">
Ok.<br class="">
<br class="">
Thanks!<br class="">
<br class="">
Jiangli<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
Thanks!<br class="">
- Ioi<br class="">
<br class="">
On 7/27/17 1:37 PM, Jiangli Zhou wrote:<br class="">
<blockquote type="cite" class="">Sorry, the mail
didn’t handle the rich text well. I fixed the
format below.<br class="">
<br class="">
Please help review the changes for JDK-8179302
(Pre-resolve constant pool string entries and
cache resolved_reference arrays in CDS
archive). Currently, the CDS archive can
contain cached class metadata, interned
java.lang.String objects. This RFE adds
the constant pool ‘resolved_references’ arrays
(hotspot specific) to the archive for
startup/runtime performance enhancement.
The ‘resolved_references' arrays are used to
hold references of resolved constant pool
entries including Strings, mirrors, etc. With
the 'resolved_references’ being cached, string
constants in shared classes can now be
resolved to existing interned
java.lang.Strings at CDS dump time. G1 and
64-bit platforms are required.<br class="">
<br class="">
The GC changes in the RFE were discussed and
guided by Thomas Schatzl and GC team. Part of
the changes were contributed by Thomas
himself.<br class="">
RFE: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8179302" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8179302</a><br class="">
hotspot: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejiangli/8179302/webrev.hotspot.01/" moz-do-not-send="true">http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.01/</a><br class="">
whitebox: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ejiangli/8179302/webrev.whitebox.01/" moz-do-not-send="true">http://cr.openjdk.java.net/~jiangli/8179302/webrev.whitebox.01/</a><br class="">
<br class="">
Please see below for details of supporting
cached ‘resolved_references’ and pre-resolving
string constants.<br class="">
<br class="">
Types of Pinned G1 Heap Regions<br class="">
<br class="">
The pinned region type is a super type of all
archive region types, which include the open
archive type and the closed archive type.<br class="">
<br class="">
00100 0 [ 8] Pinned Mask<br class="">
01000 0 [16] Old Mask<br class="">
10000 0 [32] Archive Mask<br class="">
11100 0 [56] Open Archive: ArchiveMask |
PinnedMask | OldMask<br class="">
11100 1 [57] Closed Archive: ArchiveMask |
PinnedMask | OldMask + 1<br class="">
<br class="">
<br class="">
Pinned Regions<br class="">
<br class="">
Objects within the region are 'pinned', which
means GC does not move any live objects. GC
scans and marks objects in the pinned region
as normal, but skips forwarding live objects.
Pointers in live objects are updated. Dead
objects (unreachable) can be collected and
freed.<br class="">
<br class="">
Archive Regions<br class="">
<br class="">
The archive types are sub-types of 'pinned'.
There are two types of archive region
currently, open archive and closed archive.
Both can support caching java heap objects via
the CDS archive.<br class="">
<br class="">
An archive region is also an old region by
design.<br class="">
<br class="">
Open Archive (GC-RW) Regions<br class="">
<br class="">
Open archive region is GC writable. GC scans
& marks objects within the region and
adjusts (updates) pointers in live objects
the same way as a pinned region. Live objects
(reachable) are pinned and not forwarded by
GC.<br class="">
Open archive region does not have 'dead'
objects. Unreachable objects are 'dormant'
objects. Dormant objects are not collected and
freed by GC.<br class="">
<br class="">
Adjustable Outgoing Pointers<br class="">
<br class="">
As GC can adjust pointers within the live
objects in open archive heap region, objects
can have outgoing pointers to another
java heap region, including closed archive
region, open archive region, pinned (or
humongous) region, and normal generational
region. When a referenced object is moved by
GC, the pointer within the open archive region
is updated accordingly.<br class="">
<br class="">
Closed Archive (GC-RO) Regions<br class="">
<br class="">
The closed archive region is GC read-only
region. GC cannot write into the region.
Objects are not scanned and marked by
GC. Objects are pinned and not forwarded.
Pointers are not updated by GC either. Hence,
objects within the archive region cannot
have any outgoing pointers to another java
heap region. Objects however can still have
pointers to other objects within the
closed archive regions (we might allow
pointers to open archive regions in the
future). That restricts the type of java
objects that can be supported by the archive
region.<br class="">
In JDK 9 we support archive Strings with the
archive regions.<br class="">
<br class="">
The GC-readonly archive region makes java heap
memory sharable among different JVM processes.
NOTE: synchronization on the objects within
the archive heap region can still cause writes
to the memory page.<br class="">
<br class="">
Dormant Objects<br class="">
<br class="">
Dormant objects are unreachable java objects
within the open archive heap region.<br class="">
A java object in the open archive heap region
is a live object if it can be reached during
scanning. Some of the java objects in
the region may not be reachable during
scanning. Those objects are considered as
dormant, but not dead. For example, a
constant pool 'resolved_references' array is
reachable via the klass root if its container
klass (shared) is already loaded at the time
during GC scanning. If a shared klass is not
yet loaded, the klass root is not scanned and
it's constant pool 'resolved_reference' array
(A) in the open archive region is not
reachable. Then A is a dormant object.<br class="">
<br class="">
Object State Transition<br class="">
<br class="">
All java objects are initially dormant objects
when open archive heap regions are mapped to
the runtime java heap. A dormant object
becomes live object when the associated shared
class is loaded at runtime. Explicit call
to G1SATBCardTableModRefBS::enqueue() needs to
be made when a dormant object becomes live.
That should be the case for cached objects
with strong roots as well, since strong roots
are only scanned at the start of GC marking
(the initial marking) but not during
Remarking/Final marking. If a cached object
becomes live during concurrent marking phase,
G1 may not find it and mark it live unless a
call to G1SATBCardTableModRefBS::enqueue() is
made for the object.<br class="">
<br class="">
Currently, a live object in the open archive
heap region cannot become dormant again. This
restriction simplifies GC requirement and
guarantees all outgoing pointers are updated
by GC correctly. Only objects for shared
classes from the builtin class loaders (boot,
PlatformClassLoaders, and AppClassLoaders) are
supported for caching.<br class="">
<br class="">
Caching Java Objects at Archive Dump Time<br class="">
<br class="">
The closed archive and open archive regions
are allocated near the top of the dump time
java heap. Archived java objects are copied
into the designated archive heap regions. For
example, String objects and the underlying
'value' arrays are copied into the closed
archive regions. All references to the
archived objects (from shared class metadata,
string table, etc) are set to the new heap
locations. A hash table is used to keep track
of all archived java objects during the
copying process to make sure java object is
not archived more than once if reached from
different roots. It also makes sure references
to the same archived object are updated using
the same new address location.<br class="">
<br class="">
Caching Constant Pool resolved_references
Array<br class="">
<br class="">
The 'resolved_references' is an array that
holds references of resolved constant pool
entries including Strings, mirrors
and methodTypes, etc. Each loaded class has
one 'resolved_references' array (in
ConstantPoolCache). The
'resolved_references' arrays are copied into
the open archive regions during dump process.
Prior to copying the 'resolved_references'
arrays, JVM iterates through constant pool
entries and resolves all JVM_CONSTANT_String
entries to existing interned Strings for all
archived classes. When resolving, JVM only
looks up the string table and finds existing
interned Strings without inserting new ones.
If a string entry cannot be resolved to an
existing interned String, the constant pool
entry remain as unresolved. That prevents
memory waste if a constant pool string entry
is never used at runtime.<br class="">
<br class="">
All String objects referenced by the string
table are copied first into the closed archive
regions. The string table entry is
updated with the new location when each String
object is archived. The JVM updates the
resolved constant pool string entries with the
new object locations when copying the
'resolved_references' arrays to the open
archive regions. References to
the 'resolved_references' arrays in the
ConstantPoolCache are also updated.<br class="">
At runtime as part of
ConstantPool::restore_unshareable_info() work,
call G1SATBCardTableModRefBS::enqueue() to let
GC know the 'resolved_references' is becoming
live. A handle is created for the cached
object and added to the loader_data's handles.<br class="">
<br class="">
Runtime Java Heap With Cached Java Objects<br class="">
<br class="">
<br class="">
The closed archive regions (the string
regions) and open archive regions are mapped
to the runtime java heap at the same
offsets as the dump time offsets from the
runtime java heap base.<br class="">
<br class="">
Preliminary test execution and status:<br class="">
<br class="">
JPRT: passed<br class="">
Tier2-rt: passed<br class="">
Tier2-gc: passed<br class="">
Tier2-comp: passed<br class="">
Tier3-rt: passed<br class="">
Tier3-gc: passed<br class="">
Tier3-comp: passed<br class="">
Tier4-rt: passed<br class="">
Tier4-gc: passed<br class="">
Tier4-comp:6 jobs timed out, all other tests
passed<br class="">
Tier5-rt: one test failed but passed when
running locally, all other tests passed<br class="">
Tier5-gc: passed<br class="">
Tier5-comp: running<br class="">
hotspot_gc: two jobs timed out, all other
tests passed<br class="">
hotspot_gc in CDS mode: two jobs timed out,
all other tests passed<br class="">
vm.gc: passed<br class="">
vm.gc in CDS mode: passed<br class="">
Kichensink: passed<br class="">
Kichensink in CDS mode: passed<br class="">
<br class="">
Thanks,<br class="">
Jiangli<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</div>
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
<br class="">
</div>
</div></blockquote></div><br class=""></div></body></html>