<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
These incremental changes look good to me.<br>
Thanks,<br>
Coleen<br>
<br>
<div class="moz-cite-prefix">On 8/10/17 7:51 PM, Jiangli Zhou wrote:<br>
</div>
<blockquote type="cite"
cite="mid:BB87F176-D7AF-4CAE-92AF-E126A441DE36@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
Thanks, Ioi!
<div class=""><br class="">
</div>
<div class="">Jiangli</div>
<div class=""><br class="">
<div>
<blockquote type="cite" class="">
<div class="">On Aug 10, 2017, at 2:15 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="">
<p class="">Hi Jiangli,</p>
<p class=""><br class="">
</p>
<p class="">The changes look good to me. Thanks for
considering my suggestions.</p>
<p class=""><br class="">
</p>
<p class="">- Ioi<br class="">
</p>
<br class="">
<div class="moz-cite-prefix">On 8/8/17 5:33 PM, Jiangli
Zhou wrote:<br class="">
</div>
<blockquote
cite="mid:4DEE0393-59DA-4E01-ACDB-F2DB0F9ED6D9@oracle.com"
type="cite" class="">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8" class="">
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 class="">
<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 class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
<br>
</body>
</html>