RFR (XXS): 7110173: GCNotifier::pushNotification publishes stale data.

[John Cuthbertson]

Hi David,

Thanks for the review. I did and I did that consciously. As you say - 
with the current code the listener would only be notified if the 
_recordPostGCUsage field of the TraceMemoryManagerStats object is true 
(which it is by default) and none of the TraceMemoryManagerStats 
instances created by the collectors change this. But there's nothing to 
stop a collector creating a TraceMemoryManagerStats object with 
_recordPostGCUsage false and_recordGCEndTime true. In this case wouldn't 
we still want to notify the listener? I may be wrong (in which case I'll 
add the extra guard) but I believe that recording (some) data and 
notification should be two independent operations.

Thanks,

JohnC

On 11/14/11 22:35, David Holmes wrote:
> Hi John,
>
> On 15/11/2011 4:46 AM, John Cuthbertson wrote:
>> Can I have a couple of volnteers to review the fix for this CR? The
>> webrev can be found at:
>> http://cr.openjdk.java.net/~johnc/7110173/webrev.0/.
>
> In the original code the pushNotification is conditional on 
> recordPostGCUsage, but you've removed that guard by moving the code.
>
> David
> -----
>
>> The issue here was that the routine GCNotifier::pushNotification(),
>> which uses GC data held in GCMemoryManager::_last_gc_stat, was being
>> called before the values in GCMemoryManager::_last_gc_stat were being
>> populated for the current GC. As a result the JVM could pass
>> uninitialized or stale data to a listener. The fix is to move the call
>> to GCNotifier::pushNotification() after the code that populates
>> GCMemoryManager::_last_gc_stat. I also modified the GCStatInfo
>> constructor to fully initialize instances of that class.
>>
>> Testing: The supplied test case on Windows, a crafted test case on
>> Solaris, and the nsk GC tests.
>>
>> Thanks,
>>
>> JohnC