<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Thomas,<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1/src/share/vm/gc/g1/g1CollectedHeap.cpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1/src/share/vm/gc/g1/g1CollectedHeap.cpp.frames.html</a><br>
<br>
For an evacuation failure the region is set to old here<br>
<br>
<pre>4664 // The region is now considered to be old.
<span class="changed">4665 r->set_old();
</span></pre>
and below the elapsed times are incremented.<br>
<br>
<pre><span class="changed">4676 if (r->is_young()) {</span>
<span class="changed">4677 _young_time += os::elapsedTime() - start_time;</span>
<span class="changed">4678 } else {</span>
<span class="changed">4679 _non_young_time += os::elapsedTime() - start_time;</span>
<span class="changed">4680 }
</span></pre>
<span class="changed"> Is it correct that elapsed time for a failed
evacuation will be now be added<br>
to the _non_young_time? Whereas before it was included in the
_young_time<br>
because non_young was assigned a value before the evacuation
failure was<br>
tested?<br>
<br>
Old code is<br>
<br>
</span><br>
<pre><span class="changed">4811 if (non_young) {</span>
<span class="changed">4812 non_young_time_ms += elapsed_ms;</span>
<span class="changed">4813 } else {</span>
<span class="changed">4814 young_time_ms += elapsed_ms;</span>
<span class="changed">4815 }</span></pre>
<span class="changed"><br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1/src/share/vm/gc/g1/g1CollectionSet.cpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1/src/share/vm/gc/g1/g1CollectionSet.cpp.frames.html</a><br>
<br>
</span>
<pre><span class="new"> 177 void G1CollectionSet::iterate_from(HeapRegionClosure* cl, uint worker_id, uint total_workers, bool may_be_aborted) {</span></pre>
<br>
<span class="changed"></span>I don't understand this iterate
method. It seems like because of line 196<br>
<br>
<pre><span class="new"> 195 if (cur_pos == len) {</span>
<span class="new"> 196 cur_pos = 0;</span>
<span class="new"> 197 }</span></pre>
<br>
all the workers will process region 0 in the collection set. Is
this method<br>
creating slices for each worker? Or trying to stride through the
collection<br>
set with each worker getting a different starting index?<br>
<br>
Jon<br>
<pre><span class="changed"></span></pre>
<br>
<div class="moz-cite-prefix">On 6/28/2016 5:19 AM, Thomas Schatzl
wrote:<br>
</div>
<blockquote cite="mid:1467116399.2435.18.camel@oracle.com"
type="cite">
<pre wrap="">Hi all,
On Mon, 2016-06-27 at 14:19 +0200, Thomas Schatzl wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Â Hi all,
Â
  can I have reviews for the following change that changes the
 collection set to use an external array of region ids instead of
 linking them through the HeapRegion class.
Â
</pre>
</blockquote>
<pre wrap="">
 I got some feedback from Erik Helin about this change. These are
- we do not really need the multiple-writer/multiple-reader
implementation to store the list of collection set regions. We only
ever have one concurrent reader and one concurrent writer to it.
So I simplified the data structure to use only storestore/loadload
barriers for synchronization. The relevant changes got so small that an
extra data structure was not warranted, so I removed it.
- I strengthened (added) some asserts about some methods that should
only be called by the vm thread at a safepoint to support this. Just to
indicate that in these methods we do not need the memory barriers.
- fixed a bug in the collection set iteration where the
HeapRegionClosure::incomplete() method has not been called if the
iteration was aborted.
- some cleanup
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1">http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1</a> (full)
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8159978/webrev.0_to_1/">http://cr.openjdk.java.net/~tschatzl/8159978/webrev.0_to_1/</a>Â (diff
Testing:
jprt
Thanks,
 Thomas
</pre>
</blockquote>
<br>
</body>
</html>