Request for review (XXS): 7067973: test/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java hanging intermittently

[Bengt Rutisson]

John and Jon,

Thanks for looking at this!

Here is an updated webrev based on your feedback:
http://cr.openjdk.java.net/~brutisso/7173460/webrev.01/

I added the comment that both of you suggested (I took John's wording). 
I also made a similar change 
G1CollectedHeap::do_collection_pause_at_safepoint(). Good catch, John!

Thanks again for looking at this!
Bengt


On 2012-06-02 02:17, John Cuthbertson wrote:
> Hi Bengt,
>
> The change looks OK to me. I do agree with Jon that a more explanatory 
> comment is needed. Perhaps something along the lines:
>
> // We must call G1MonitoringSupport::update_sizes() in the same 
> scoping level
> // as an active TraceMemoryManagerStats object (i.e. before the 
> destructor for the
> // TraceMemoryManagerStats is called) so that the G1 memory pools are 
> updated
> // before any GC notifications are raised.
>
> However, I think that 
> G1CollectedHeap::do_collection_pause_at_safepoint() may have the same 
> issue.
>
> The scope in which the TraceMemoryManagerStats object is created is at 
> line 3612:
>
>  // Inner scope for scope based logging, timers, and stats collection
>  {
>    ...
>    TraceCollectorStats tcs(g1mm()->incremental_collection_counters());
>    TraceMemoryManagerStats tms(false /* fullGC */, gc_cause());
>
> The TraceMemoryManagerStats object is created at line 3629 (above).
>
> The scope is closed at line 3933:
>
>      gc_epilogue(false);
>    }
>  }
>
>  // The closing of the inner scope, immediately above, will complete
>  // logging at the "fine" level. The record_collection_pause_end() call
>  // above will complete logging at the "finer" level.
>
> The update_sizes() call is at line 3951:
>
>  print_heap_after_gc();
>  g1mm()->update_sizes();
>
> It should be safe to move these two lines into the scope.
>
> A possible way to test this could be to run the test with 
> -XX:+ExplicitGCInvokesConcurrent.
>
> Regards,
>
> JohnC
>
> On 05/31/12 13:04, Bengt Rutisson wrote:
>>
>> Hi all,
>>
>> Can I have a couple of reviews for this really small change?
>> http://cr.openjdk.java.net/~brutisso/7067973/webrev.00/
>>
>> Background:
>> The CollectionUsageThreshold test fails with G1. The test lowers the 
>> notification threshold for the G1 old gen memory pool and expects to 
>> get a notification after a full GC.
>>
>> The problem in G1 is that the decision to send the notification is 
>> done in TraceMemoryManagerStats::~TraceMemoryManagerStats(). This 
>> eventually does pool->get_memory_usage() to get the memory usage 
>> after a collection. The problem is that we update this information in 
>> G1MonitoringSupport::update_sizes() which is called in 
>> G1CollectedHeap::do_collection() _after_ the TraceMemoryManagerStats 
>> scope had been exited.
>>
>> Extending the scope to cover the call to 
>> G1MonitoringSupport::update_sizes() solves the issue.
>>
>> Testing:
>> Before this change the CollectionUsageThreshold failed every time I 
>> ran it. After this change it passes every time I ran it.
>>
>> Thanks,
>> Bengt
>