<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
John,<br>
<br>
Looks good to me.<br>
<br>
One minor thing. I think the two asserts on lines 2710 and 2711 are
unnecessary. They will never fail.<br>
<br>
2709 HeapRegion* G1CollectedHeap::start_cset_region_for_worker(int
worker_i) {<br>
2710 assert(_worker_cset_start_region != NULL, "sanity");<br>
2711 assert(_worker_cset_start_region_time_stamp != NULL,
"sanity");<br>
<br>
In the constructor for G1CollectedHeap you set the arrays up using
NEW_C_HEAP_ARRAY. If that fails to allocate memory it will call
vm_exit_out_of_memory(). So, you should never get to
start_cset_region_for_worker() with the arrays being NULL.<br>
<br>
Bengt<br>
<br>
<br>
On 2011-12-14 23:23, John Cuthbertson wrote:
<blockquote cite="mid:4EE921CD.1010301@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
Hi Everyone,<br>
<br>
Based upon some further comments from Tony, I have another new
webrev
for this change. It can be found at:
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/7119908/webrev.2/">http://cr.openjdk.java.net/~johnc/7119908/webrev.2/</a><br>
<br>
Changes in this version include:<br>
<br>
* some minor name changes<br>
* the code that calculates the starting region was using
total_workers() when it should have been using active_workers()<br>
* minor tweaks to the start and end index values to get a slightly
better distribution.<br>
<br>
Testing:<br>
GC test suite<br>
<br>
Thanks,<br>
<br>
JohnC<br>
<br>
On 12/13/11 10:39, John Cuthbertson wrote:
<blockquote cite="mid:4EE79BF4.2040809@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Hi Everyone,<br>
<br>
I have a new webrev based upon comments from Tony, which can be
found
at: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/7119908/webrev.1/">http://cr.openjdk.java.net/~johnc/7119908/webrev.1/</a><br>
<br>
Changes in this version include:<br>
<br>
* Using the value of the existing
G1CollectedHeap::_gc_time_stamp
field, rather than the value of _total_collections, as the TS
values.<br>
* A small optimization that may reduce the number of iterations
while
walking the collection set to calculate the starting region for
a
worker. We check the entry for the previous worker and if that
is valid
then we starting iterating over the collection set from that
region.<br>
<br>
Testing:<br>
GC test suite; jprt.<br>
<br>
Thanks,<br>
<br>
JohnC<br>
<br>
On 12/09/11 11:18, John Cuthbertson wrote:
<blockquote cite="mid:4EE25EFE.9090902@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
Hi Everyone,<br>
<br>
Can I have a couple of volunteers to review the changes for
this CR?
The webrev can be found at: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejohnc/7119908/webrev.0/">http://cr.openjdk.java.net/~johnc/7119908/webrev.0/</a><br>
<br>
Summary:<br>
As part of the code review comments for 7112743 (G1: Reduce
overhead of
marking closure during evacuation pauses) it was suggested
that instead
of recalculating the starting heap region for each worker
thread, we
reuse the values calculated during RSet scanning. These
changes address
that review comment. In these changes I maintain a cache that
is used
and updated by
G1CollectedHeap::start_cset_region_for_worker(). The
first time this routine is called by a worker thread during an
evacuation pause (currently during RSet scanning) the cached
value for
the worker gets set; when the routine is called subsequently
the region
that was cached for the worker is returned. I employ a simple
stamp
mechanism based upon the number of GCs that ensures the
validity of the
regions in the cache and makes clearing the cache unnecessary.<br>
<br>
Testing:<br>
GC Test suite with a small marking threshold (10%) with and
without
verification; the configuration of SPECjbb used to test
7117423;
Kitchensink.<br>
<br>
Thanks,<br>
<br>
JohnC<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>