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

[Jon Masamitsu]

Bengt,

Extension of the second scope to fix a similar problem looks
fine.  Ship it.

Jon

On 6/4/2012 4:33 AM, Bengt Rutisson wrote:
>
> 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
>>
>