RFR (XXS): 7110173: GCNotifier::pushNotification publishes stale data.
John Cuthbertson
john.cuthbertson at oracle.com
Wed Nov 16 17:40:19 UTC 2011
Hi Fred,
Thanks for the review and explanation. As for being an 'official'
reviewer - 'official' is in the eye of the beholder. Since you have been
looking at this code recently, you and your comments carry more weight.
JohnC
On 11/16/2011 2:00 AM, Frederic Parain wrote:
> Hi,
>
> The reason why the memory usage data are bundled with the notification
> is to prevent data lost. If the notification doesn't provide the
> memory usage data, the client has to perform a getLastGcInfo() call
> to retrieve the data each time it receives a notification. But several
> GC cycles might occur between the time the notification is received and
> the time getLastGcInfo() is invoked. And there's no API to get memory
> usage data older than the one from the last GC cycle. With the
> notification including the memory usage data, the client is ensured
> to get data for all GC cycles.
>
> That said, the notification contains two memory usage datasets, one
> created at the beginning of the GC cycle and one created at the end
> of the cycle. There's no reason to condition the notification to
> the recording of the post GC cycle data recording. It's reasonable
> to send a notification for a GC which only collects the pre GC data
> for instance. So I think it's ok to move the notification code out
> of the recordPostGCUsage==true condition as long as the values in
> the non recorded memory usage data set can clearly be identified
> as being invalid. Right now they are initialized to zero, so it
> looks ok to me.
>
> Moving the notification code after the "if(countCollection)" block
> looks good too.
>
> However, I'm not an official reviewer, so you'll need more approvals
> for this changeset.
>
> Fred
>
> On 11/16/11 03:26 AM, Mandy Chung wrote:
>> I'm including Frederic and serviceability-dev since this is part of the
>> instrumentation done for serviceability.
>>
>> On 11/15/2011 4:59 PM, David Holmes wrote:
>>> 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.
>>>
>>
>> The GC notifier and pushNotification call was added as this changeset:
>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/78542e2b5e35
>>
>> The spec for this is:
>> http://download.oracle.com/javase/7/docs/jre/api/management/extension/com/sun/management/GarbageCollectionNotificationInfo.html
>>
>>
>>
>>> 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.
>>>
>>
>> I think David is right that it should check recordPostGCUsage==true to
>> push a notification. The notification is sent with the last GC
>> statistics. If my memory serves correctly, if recordPostGCUsage is
>> false, it doesn't update the last GC usage but sending a notification
>> when recordPostGCUsage is false seems to be incorrect. Also, the
>> recording was specifically added for CMS since CMS has separate phases
>> that it needs to record different things.
>>
>> Frederic would be the best person to comment on this. Frederic - it
>> seems to me that the pushNotification can be moved to the end of gc_end
>> function but within the if (countCollection) statement. Sorry I don't
>> have time to check the details out. It'd be good if you can help.
>>
>> Thanks
>> Mandy
>>
>>> 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
>>>>
>
More information about the hotspot-gc-dev
mailing list