Request for review (S): 7152954 G1: Native memory leak during full GCs

[John Cuthbertson]

Hi Bengt,

Looks good  - but I do have a couple of comments:

survRateGroup.cpp:

This is not related to your changes - but I just noticed it:

 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     }

should be:

 108     _surv_rate_pred = NEW_C_HEAP_ARRAY(TruncatedSeq*, _region_num);
 109     if (*_surv_rate_pred* == NULL) {
 110       vm_exit_out_of_memory(sizeof(TruncatedSeq*) * _region_num,
 111                             "Not enough space for surv rate pred array.");
 112     }

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:

 117     for (size_t i = _stats_arrays_length; i < _prev_stats_arrays_length; ++i) {
 118       delete old_surv_rate_pred[i];
 119     }

really look like they belong in SurvRateGroup::reset().

Excellent catch - what brought it to your attention?

JohnC


On 03/12/12 02:36, Bengt Rutisson wrote:
>
> Hi all,
>
> Could I have a couple of reviews for this fairly small change:
>
> http://cr.openjdk.java.net/~brutisso/7152954/webrev.00/
>
> The CR will be publicly available soon here:
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7152954
>
>
> 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.
>
>
> Background from the CR:
>
> There is a memory leak in the full GC code for G1. This can be seen by 
> running this simple reproducer:
>
> public class SysGC {
>     public static void main(String[] args) {
>         while (true) {
>             System.gc();
>         }
>     }
> }
>
> I run it with this command line:
>
> java -XX:+UseG1GC -Xms16m -Xmx16m -XX:+PrintGC SysGC
>
> Watching the memory footprint for the java process shows that it is 
> constantly using more memory.
>
> 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().
>
> The problem with SurvRateGroup::stop_adding_regions() is that it does:
>
> _surv_rate_pred[i] = new TruncatedSeq(10);
>
> in a loop every time it is called. But there is no corresponding call 
> to delete.
>
> 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.
>
> With these two fixes the memory leak seems to go away.
>
> Thanks,
> Bengt

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120312/a553c025/attachment.htm>