<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Thomas,<br>
<br>
On 4/14/16 6:08 AM, Thomas Schatzl wrote:<br>
</div>
<blockquote cite="mid:1460628517.2186.19.camel@oracle.com"
type="cite">
<pre wrap="">Hi Kim,
thanks for your review.
On Thu, 2016-04-14 at 00:31 -0400, Kim Barrett wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">On Apr 13, 2016, at 11:42 AM, Thomas Schatzl <
<a class="moz-txt-link-abbreviated" href="mailto:thomas.schatzl@oracle.com">thomas.schatzl@oracle.com</a>> wrote:
Hi all,
</pre>
</blockquote>
</blockquote>
<pre wrap="">[...]
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">CR:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8153170">https://bugs.openjdk.java.net/browse/JDK-8153170</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8153170/webrev/">http://cr.openjdk.java.net/~tschatzl/8153170/webrev/</a>
Testing:
jprt, vm.gc
</pre>
</blockquote>
<pre wrap="">
I think this looks ok.
My one concern is that I'm having trouble figuring out when a heap
region might have reset_gc_time_stamp called on it. Can that happen
in such a way that it would interfere with the use of the time_stamp
being added by these changes? I will continue to study, but maybe
you already know and can provide guidance.
</pre>
</blockquote>
<pre wrap="">
I do not think so.
Global reset_gc_time_stamp() (reset_gc_time_stamp() called on
G1CollectedHeap and all regions) is called during full gc (which aborts
getting live data anyway), and at the end of the cleanup pause, after
finalizing live data.
So the reset of the gc time stamps should be safe and not interfere
with this use.
</pre>
<blockquote type="cite">
<pre wrap="">Also a couple of minor things:
---------------------------------------------------------------------
---------
src/share/vm/gc/g1/g1CardLiveData.hpp
49 // While concurrently creating live data regions may be
reclaimed (e.g. humongous
I keep having a hard time reading this because I see
"live data regions"
and wondering what those might be. How about
Regions may be reclaimed while concurrently creating live data
---------------------------------------------------------------------
---------
src/share/vm/gc/g1/g1CardLiveData.cpp
Several logging statements added, but this file doesn't #include
logging.
</pre>
</blockquote>
<pre wrap="">---------
Now we know why the previous CR had a superfluous #include right
there...
Thanks a lot for catching these.
New webrevs at
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8153170/webrev.0_to_1/">http://cr.openjdk.java.net/~tschatzl/8153170/webrev.0_to_1/</a> (diff)
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8153170/webrev.1/">http://cr.openjdk.java.net/~tschatzl/8153170/webrev.1/</a> (full)
Thanks,
Thomas
</pre>
</blockquote>
<br>
This looks good. A couple of minor items. <br>
<br>
g1CardLiveData.cpp:<br>
<br>
- Line 488 vs. (496, 512).<br>
The comment at 488 doesn't match the code. The comment talks about
the old (expected && !actual) condition. Is (!expected
&& actual) somehow an OK condition or a failure? Or is the
change part of having more exact card liveness information?
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<br>
<br>
heapRegion.hpp<br>
- Copyright.<br>
<br>
I don't need to see an updated webrev though.<br>
- Derek<br>
</body>
</html>