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

[Bengt Rutisson]

Thanks, Jon!

I'll go ahead and push this.

Bengt

On 2012-06-05 22:33, Jon Masamitsu wrote:
> 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
>>>
>>