<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
Hi Bengt,<br>
<br>
Looks good - but I do have a couple of comments:<br>
<br>
survRateGroup.cpp:<br>
<br>
This is not related to your changes - but I just noticed it:<br>
<br>
<pre> 108 _surv_rate_pred = NEW_C_HEAP_ARRAY(TruncatedSeq*, _region_num);
109 if (_surv_rate == NULL) {
110 vm_exit_out_of_memory(sizeof(TruncatedSeq*) * _region_num,
111 "Not enough space for surv rate pred array.");
112 }
</pre>
should be:<br>
<br>
<pre> 108 _surv_rate_pred = NEW_C_HEAP_ARRAY(TruncatedSeq*, _region_num);
109 if (<b>_surv_rate_pred</b> == NULL) {
110 vm_exit_out_of_memory(sizeof(TruncatedSeq*) * _region_num,
111 "Not enough space for surv rate pred array.");
112 }
</pre>
My picture of SurvRateGroup::stop_adding_regions() has always been that
it's just extending the stats arrays (i.e., a realloc operation). So,
with that picture in mind, the following lines:<br>
<br>
<pre><span class="changed"> 117 for (size_t i = _stats_arrays_length; i < _prev_stats_arrays_length; ++i) {</span>
<span class="changed"> 118 delete old_surv_rate_pred[i];</span>
<span class="changed"> 119 }</span>
</pre>
really look like they belong in SurvRateGroup::reset().<br>
<br>
Excellent catch - what brought it to your attention?<br>
<br>
JohnC<br>
<br>
<br>
On 03/12/12 02:36, Bengt Rutisson wrote:
<blockquote cite="mid:4F5DC3A1.1050306@oracle.com" type="cite"><br>
Hi all,
<br>
<br>
Could I have a couple of reviews for this fairly small change:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/7152954/webrev.00/">http://cr.openjdk.java.net/~brutisso/7152954/webrev.00/</a>
<br>
<br>
The CR will be publicly available soon here:
<br>
<a class="moz-txt-link-freetext" href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7152954">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7152954</a>
<br>
<br>
<br>
I choose to do a fairly small fix. To me it looks like the
SurvRateGroup class could benefit from some refactoring. Maybe this
hole issue could go away with some simplifications to the code. But I'd
like to keep the change as small as possible for now just in case we
would like to push this into hs23.
<br>
<br>
<br>
Background from the CR:
<br>
<br>
There is a memory leak in the full GC code for G1. This can be seen by
running this simple reproducer:
<br>
<br>
public class SysGC {
<br>
public static void main(String[] args) {
<br>
while (true) {
<br>
System.gc();
<br>
}
<br>
}
<br>
}
<br>
<br>
I run it with this command line:
<br>
<br>
java -XX:+UseG1GC -Xms16m -Xmx16m -XX:+PrintGC SysGC
<br>
<br>
Watching the memory footprint for the java process shows that it is
constantly using more memory.
<br>
<br>
The leak comes from SurvRateGroup::stop_adding_regions() which is
called from SurvRateGroup::reset(), which in turn is called from
G1CollectorPolicy::record_full_collection_end().
<br>
<br>
The problem with SurvRateGroup::stop_adding_regions() is that it does:
<br>
<br>
_surv_rate_pred[i] = new TruncatedSeq(10);
<br>
<br>
in a loop every time it is called. But there is no corresponding call
to delete.
<br>
<br>
Adding a loop to call delete on the previously allocated TruncatedSeq
objects is not enough to solve the problem since TruncatedSeq is itself
allocating an array without freeing it. Adding a destructor to
TruncatedSeq that frees the allocated array solves the issue.
<br>
<br>
With these two fixes the memory leak seems to go away.
<br>
<br>
Thanks,
<br>
Bengt
<br>
</blockquote>
<br>
</body>
</html>