<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Thanks John and Tony for looking at this.<br>
<br>
I'll go ahead and push this.<br>
<br>
Bengt<br>
<br>
On 2012-03-13 22:20, Tony Printezis wrote:
<blockquote cite="mid:4F5FBA15.3020501@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Bengt,<br>
<br>
Thanks for doing the extra cleanup! The new version looks fine.<br>
<br>
Tony<br>
<br>
On 03/13/2012 04:30 PM, Bengt Rutisson wrote:
<blockquote cite="mid:4F5FAE72.7020602@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
Tony,<br>
<br>
Thanks for the review! And thanks for being picky :-)<br>
<br>
Updated webrev based on your comments:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/7152954/webrev.02/">http://cr.openjdk.java.net/~brutisso/7152954/webrev.02/</a><br>
<br>
Bengt<br>
<br>
<br>
On 2012-03-13 17:10, Tony Printezis wrote:
<blockquote cite="mid:4F5F716E.9080701@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Bengt,<br>
<br>
First, very good catch! And I agree with you that a)
SurvRateGroup does need some tidying up and b) you're taking
the right approach in keeping this fix simple at this time.<br>
<br>
The fix looks great, some minor comments below:<br>
<br>
<pre><span class="changed"> 41 _surv_rate_pred(NULL),</span>
<span class="changed"> 42 _stats_arrays_length(0)</span>
43 {
44 reset();
45 if (summary_surv_rates_len > 0) {
46 size_t length = summary_surv_rates_len;
47 _summary_surv_rates = NEW_C_HEAP_ARRAY(NumberSeq*, length);
48 if (_summary_surv_rates == NULL) {
49 vm_exit_out_of_memory(sizeof(NumberSeq*) * length,
50 "Not enough space for surv rate summary");
51 }
52 for (size_t i = 0; i < length; ++i)
53 _summary_surv_rates[i] = new NumberSeq();
54 }
</pre>
Since you removed the NULL check in the other places where
NEW_C_HEAP_ARRAY() is called, any chance of doing that here
too to be consistent throughout the file? Also, (and this is
really picky!) could you move the curly bracket on line 43 to
the line above (you did in the reset() method) and put curly
brackets around the body of the for-loop? Thanks!<br>
<br>
<pre><span class="removed"> 92 #if 0</span>
<span class="removed"> 93 gclog_or_tty->print_cr("[%s] stop adding regions, length %d", _name, _region_num);</span>
<span class="removed"> 94 #endif // 0</span>
<span class="removed"></span></pre>
Since you removed this, any chance of maybe removing the other
instances of #if 0 gclog_or_tty->print_cr(...); #endif //
0?<br>
<br>
On 03/13/2012 04:49 AM, Bengt Rutisson wrote:
<blockquote cite="mid:4F5F0A10.7050301@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
Hi John,<br>
<br>
Thanks for the review!<br>
<br>
Here is an updated webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/7152954/webrev.01/">http://cr.openjdk.java.net/~brutisso/7152954/webrev.01/</a><br>
<br>
Some comments inline...<br>
<br>
On 2012-03-13 00:03, John Cuthbertson wrote:
<blockquote cite="mid:4F5E80DD.6030801@oracle.com"
type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
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>
</blockquote>
<br>
You are correct, but NEW_C_HEAP_ARRAY will call
vm_exit_out_of_memory() if the allocation fails. So this is
dead code. I removed it instead of updating it.<br>
<br>
<blockquote cite="mid:4F5E80DD.6030801@oracle.com"
type="cite"> 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>
</blockquote>
<br>
Good point. I was struggling a bit with how this code was
supposed to be used. I moved the delete calls into reset().
Thanks for pointing this out. <br>
<br>
<blockquote cite="mid:4F5E80DD.6030801@oracle.com"
type="cite"> Excellent catch - what brought it to your
attention?<br>
</blockquote>
<br>
It is kind of a funny story. Here is probably more
information than you want, so just stop reading if you find
it too long ;-)<br>
<br>
I just wanted to confirm that a small test I found would
eventually throw an OOME. I ran it on JDK7 b147 over a
weekend. When I checked the test after the weekend it had
gotten a native out of memory instead. The test would mostly
allocate humongous objects so I figured we were leaking in
this respect somehow.<br>
<br>
I instrumented a build from the tip of hotspot-gc. But to my
surprise the leak was gone. That's when I realized that in
b147 we were constantly doing full GCs to handle all the
humongous objects. But in the latest builds we have the fix
I made to initiate concurrent marking on humongous object
allocations, so we never did any full GCs. By inducing full
GCs with System.gc() calls I could show that the memory leak
was in fact still present in the latest builds as well.<br>
<br>
I tried out the prototype that Zhengyu has for native memory
tracking. Unfortunately I didn't get it to work properly on
my 64 bit system. After I had already found the cause of the
leak, Zhengyu ran my reproducer on a 32 bit system and got
this report:<br>
<br>
[0x54dd14fc] TruncatedSeq::TruncatedSeq+0x4c<br>
(malloc=162KB 143KB, #2071 1833)<br>
<br>
[0x54f43737] SurvRateGroup::reset+0x47<br>
(malloc=128KB 115KB, #2049 1833)
<br>
<br>
That is really spot on (assuming
SurvRateGroup::stop_adding_regions() got inlined into
SurvRateGroup::reset()). So, it looks to me as though NMT
will be a really useful tool to find this type of issues in
the future.<br>
<br>
Once I had the small reproducer that just calls System.gc()
in a loop I could manually do a binary search of the full GC
code to find out where we were leaking.<br>
<br>
Long story short: I found this by accident and NMT will be a
really good tool.<br>
<br>
Bengt<br>
<br>
<br>
<br>
<blockquote cite="mid:4F5E80DD.6030801@oracle.com"
type="cite"> <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 moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/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 moz-do-not-send="true" 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>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>