<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Bengt,<br>
Thanks very much for the review.<br>
<br>
<div class="moz-cite-prefix">On 6/1/2015 4:35 AM, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:556C196B.9000106@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Tom, <br>
<br>
(Correct email thread this time, I hope.)<br>
<br>
Thanks for providing the great summary that eased the review
process. This change looks good to me. <br>
<br>
Some minor comments: <br>
<br>
g1Allocator.cpp <br>
<br>
214 assert(_bottom >= _allocation_region->bottom(),
"inconsistent allocation state"); <br>
215 assert(_max <= _allocation_region->end(),
"inconsistent allocation state"); <br>
216 assert(_bottom <= old_top && old_top <=
_max, "inconsistent allocation state"); <br>
<br>
Maybe add err_msg() to see what the values actually were? <br>
<br>
</div>
</blockquote>
<br>
Good idea.<br>
<br>
<blockquote cite="mid:556C196B.9000106@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
g1CollectedHeap.cpp <br>
<br>
965 bool <br>
966 G1CollectedHeap::check_archive_addresses() <br>
<br>
We usually don’t have a linefeed after the return type. So, this
should be: <br>
<br>
965 bool G1CollectedHeap::check_archive_addresses() <br>
<br>
Same for G1CollectedHeap::alloc_archive_regions() and
G1CollectedHeap::fill_archive_regions() <br>
<br>
<br>
</div>
</blockquote>
<br>
OK. I made the mistake of following the example of existing
allocation routines. 8^)<br>
<br>
<blockquote cite="mid:556C196B.9000106@oracle.com" type="cite">
<div class="moz-cite-prefix"> g1MarkSweep.cpp <br>
<br>
306 void G1MarkSweep::enable_archive_object_check() { <br>
307 if (!_archive_check_enabled) { <br>
<br>
Shouldn’t this be called exactly once, either from
G1RecordingAllocator::create_allocator() or
G1CollectedHeap::alloc_archive_regions()? In that case maybe the
“if” should be changed to an assert or guarantee that
!_archive_check_enabled holds. <br>
<br>
</div>
</blockquote>
<br>
I think this is debatable, but I'll change it. As it is now,
alloc_archive_regions could be called multiple times to map multiple
archived MemRegion sets. In this current use for CDS share strings,
there should indeed be only 1 call. When I change this to an
assertion, that will be the only thing preventing multiple calls,
should another use of archive regions be added. The assertion can
be removed then.<br>
Tom<br>
<br>
<blockquote cite="mid:556C196B.9000106@oracle.com" type="cite">
<div class="moz-cite-prefix"> Thanks, <br>
Bengt <br>
<br>
<br>
On 29/05/15 23:30, Tom Benson wrote:<br>
</div>
<blockquote cite="mid:5568DA67.5010808@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
Hi,<br>
Please review these changes for JDK-8042668, which constitute
the GC support for JDK-8059092 for storing interned strings in
CDS archives (JEP 250). The RFR for JDK-8059092 was recently
posted by Jiangli Zhou, and it would be best if overall comments
could go to that thread, with GC-specific comments here.<br>
<br>
JBS: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8042668">https://bugs.openjdk.java.net/browse/JDK-8042668</a><br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/8042668/webrev.00/">http://cr.openjdk.java.net/~brutisso/8042668/webrev.00/</a><br>
<br>
These changes add a new "archive" region type to G1. The
description field in JDK-8042668 contains an "Implementation
Notes" section which describes components of the design, and
should be useful for a code review. The overview:<br>
<blockquote>"Archive" regions are G1 regions that are not
modifiable by GC, being neither scavenged nor compacted, or
even marked in the object header. They can contain no pointers
to non-archive heap regions, and object headers point to
shared CDS metaspace (though this last point is not enforced
by G1). Thus, they allow the underlying hardware pages to be
shared among multiple JVM instances. <br>
<br>
In short, a dump-time run (using -Xshare:dump) will allocate
space in the Java heap for the strings which are to be shared,
copy the string objects and arrays to that space, and then
archive the entire address range in the CDS archive. At
restore-time (using -Xshare:on), that same heap range will be
allocated at JVM init time, and the archived data will be
mmap'ed into it. GC must treat the range as 'pinned,' never
moving or writing to any objects within it, so that cross-JVM
sharing will work. <br>
</blockquote>
Testing: All testing for JDK-8059092 included this code.
Manual testing with prototype calls to the new GC support was
performed before integration, along with JPRT and benchmark
runs.<br>
<br>
Performance: The GC changes had no significant impact on
SpecJBB, JVM, or Dacapo benchmarks, run on x64 Linux. However,
a small (~1%) increase in Full GC times was seen in tests when
the shared string support was not in use, when runs are
configured to encounter them. When shared strings ARE in use,
the impact could be as high as 5% for a likely worst-case.
Please see the JBS entry for a discussion.<br>
<br>
Thanks, <br>
Tom<br>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>