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

[David Holmes]

Hi John,

On 16/11/2011 3:33 AM, John Cuthbertson wrote:
> 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.

It depends on what data the listener is expecting to receive. If you 
push the notification when there hasn't been an update does that make 
sense? I don't know what the spec is for this.

Anyway I just wanted to flag the change in potential behaviour. If 
no-one from GC has any issue with this then it's fine by me.

David
-----

>
> 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
>