<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Hi Jiangli,</p>
<p><br>
</p>
<p>The changes look good to me. Thanks for considering my
suggestions.</p>
<p><br>
</p>
<p>- Ioi<br>
</p>
<br>
<div class="moz-cite-prefix">On 8/8/17 5:33 PM, Jiangli Zhou wrote:<br>
</div>
<blockquote
cite="mid:4DEE0393-59DA-4E01-ACDB-F2DB0F9ED6D9@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
Here is the incremental webrev that has all the changes
incorporated with suggestions from Coleen, Ioi and Thomas:
<div class=""><br class="">
</div>
<div class="">
<div style="margin: 0px;" class=""><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejiangli/8179302/webrev.hotspot.03.inc/"
class="">http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.03.inc/</a></div>
<div class=""><br class="">
</div>
<div class="">Updated full webrev: <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejiangli/8179302/webrev.hotspot.03/"
class="">http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.03/</a></div>
<div class=""><br class="">
</div>
<div class="">Thanks again for Coleen's, Ioi's and Thomas’
review!</div>
<div class="">Jiangli</div>
<div class=""><br class="">
<div>
<blockquote type="cite" class="">
<div class="">On Aug 7, 2017, at 7:57 PM, Jiangli Zhou
<<a moz-do-not-send="true"
href="mailto:jiangli.zhou@Oracle.COM" class="">jiangli.zhou@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 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 class="">
<blockquote type="cite" class="">
<div class="">On Aug 7, 2017, at 5:45 PM, Ioi
Lam <<a moz-do-not-send="true"
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 class=""><br class="">
</div>
<div class="">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 class=""><br class="">
</div>
Ok.</div>
<div class=""><br class="">
</div>
<div class="">I’m updating my changes and will send
out a consolidated webrev.</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 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>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</div>
</blockquote>
<br>
</body>
</html>