Dan,
Can you possibly change the two "in a parallel" to "in parallel" ?
thanks,
Karen
On Sep 2, 2015, at 1:06 PM, Tom Benson wrote:
> Looks good to me!
> Tnx,
> Tom
>
> On 9/2/2015 12:40 PM, Daniel D. Daugherty wrote:
>> Just for the record, here are the comment context diffs:
>>
>> $ diff -c src/share/vm/runtime/perfMemory.cpp{.cr2,}***
>> src/share/vm/runtime/perfMemory.cpp.cr2 Tue Sep 1 19:39:45 2015
>> --- src/share/vm/runtime/perfMemory.cpp Wed Sep 2 09:35:48 2015
>> ***************
>> *** 70,76 ****
>> // objects that are currently being used by running JavaThreads
>> // or the StatSampler. This method is invoked while we are not at
>> // a safepoint during a VM abort so leaving the PerfData objects
>> ! // around may also help diagnose the failure.
>> //
>> if (SafepointSynchronize::is_at_safepoint() && !StatSampler::is_active())
>> {
>> PerfDataManager::destroy();
>> --- 70,78 ----
>> // objects that are currently being used by running JavaThreads
>> // or the StatSampler. This method is invoked while we are not at
>> // a safepoint during a VM abort so leaving the PerfData objects
>> ! // around may also help diagnose the failure. In rare cases,
>> ! // PerfData objects are used in parallel with a safepoint. See
>> ! // the work around in PerfDataManager::destroy().
>> //
>> if (SafepointSynchronize::is_at_safepoint() && !StatSampler::is_active())
>> {
>> PerfDataManager::destroy();
>>
>>
>> $ diff -c src/share/vm/runtime/objectMonitor.cpp{.cr2,}
>> *** src/share/vm/runtime/objectMonitor.cpp.cr2 Tue Sep 1 19:23:35 2015
>> --- src/share/vm/runtime/objectMonitor.cpp Wed Sep 2 09:37:08 2015
>> ***************
>> *** 572,577 ****
>> --- 572,579 ----
>> // That is by design - we trade "lossy" counters which are exposed to
>> // races during updates for a lower probe effect.
>> TEVENT(Inflated enter - Futile wakeup);
>> + // This PerfData object can be used in a parallel with a safepoint.
>> + // See the work around in PerfDataManager::destroy().
>> OM_PERFDATA_OP(FutileWakeups, inc());
>> ++nWakeups;
>>
>> ***************
>> *** 744,749 ****
>> --- 746,753 ----
>> // *must* retry _owner before parking.
>> OrderAccess::fence();
>>
>> + // This PerfData object can be used in a parallel with a safepoint.
>> + // See the work around in PerfDataManager::destroy().
>> OM_PERFDATA_OP(FutileWakeups, inc());
>> }
>>
>>
>> Dan
>>
>>
>> On 9/2/15 10:03 AM, Daniel D. Daugherty wrote:
>>> On 9/2/15 9:40 AM, Tom Benson wrote:
>>>> Hi Dan,
>>>> OK. I didn't review what was added in round 1 once you said it was all
>>>> removed for round 2. 8^)
>>>
>>> Not "all", but I did remove "most" of the round 1 changes :-)
>>> The changes I kept are called in the list below.
>>>
>>>
>>>> It would be great if what you have in your first paragraph below was added
>>>> to the comments. I think the existing comment in perfMemory_exit implies
>>>> we're safe to remove the PerfData objects without fear of them being in
>>>> use because we're at a safepoint.
>>>
>>> I think I'll add this sentence to the comment in perfMemory_exit():
>>>
>>> // In rare cases, PerfData objects are used in parallel with a
>>> // safepoint. See the work around in PerfDataManager::destroy().
>>>
>>>
>>>> Perhaps better to have it (the new comment) in PerfDataManager::destroy(),
>>>> because it seems so weird to have a sleep in the VM thread during a
>>>> safepoint, even in a shutdown path.
>>>
>>> I think the PerfDataManager::destroy() comment is clear about
>>> the race we're trying avoid. Again, if you have specific wording
>>> changes to suggest to make it more clear... I'll take them. :-)
>>>
>>> I think I'll also add this comment:
>>>
>>> // This PerfData object can be used in a parallel with a safepoint.
>>> // See the work around in PerfDataManager::destroy().
>>>
>>> above these lines in src/share/vm/runtime/objectMonitor.cpp:
>>>
>>> 575 OM_PERFDATA_OP(FutileWakeups, inc());
>>> 747 OM_PERFDATA_OP(FutileWakeups, inc());
>>>
>>>
>>>> Any interest in asserting that you're at a safepoint in
>>>> PerfDataManager::destroy? Just a thought.
>>>
>>> I'd rather not add an assert() at this time.
>>>
>>> Are you good with the above comment additions? Do you need to
>>> see another webrev when I make those changes?
>>>
>>> Dan
>>>
>>>
>>>> Tom
>>>>
>>>> On 9/2/2015 11:15 AM, Daniel D. Daugherty wrote:
>>>>> On 9/2/15 8:49 AM, Tom Benson wrote:
>>>>>> Hi,
>>>>>> I'm a bit confused on one point... Since you now only call
>>>>>> PerfDataManager::destroy at a safepoint, why do you still have the
>>>>>> comment about 'the race' and the sleep?
>>>>>
>>>>> Because the two "futile wakeup" counter updates in the monitor
>>>>> subsystem can execute in parallel with a safepoint. The JavaThread
>>>>> state is "blocked" so the safepoint subsystem will see the JavaThread
>>>>> as "at a safepoint" when it is actually executing the code to
>>>>> increment the counter.
>>>>>
>>>>> That's what the "is_safe" parameter to the OM_PERFDATA_OP macro was
>>>>> all about in the round 1 code review. However, David convinced me
>>>>> that all that logic didn't guarantee we wouldn't hit the race so
>>>>> I ripped it all out in the round 2 code review (this one).
>>>>>
>>>>> Does this help your confusion?
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>> Tom
>>>>>>
>>>>>> On 9/2/2015 7:52 AM, Daniel D. Daugherty wrote:
>>>>>>> David,
>>>>>>>
>>>>>>> Thanks for the very fast re-review!
>>>>>>>
>>>>>>> Enjoy your vacation!
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>> On 9/2/15 2:54 AM, David Holmes wrote:
>>>>>>>> Hi Dan,
>>>>>>>>
>>>>>>>> On 2/09/2015 2:45 PM, Daniel D. Daugherty wrote:
>>>>>>>>> Greetings,
>>>>>>>>>
>>>>>>>>> I've updated the "fix" for this bug based on code review comments
>>>>>>>>> received in round 1. I've dropped most of the changes from round 1
>>>>>>>>> with a couple of exceptions.
>>>>>>>>
>>>>>>>> I have no further comments - it all looks good to me. If others want
>>>>>>>> the pendulum to swing back a little from this position then ...
>>>>>>>> nothing that has been suggested is functionally wrong. :)
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>> PS. When you get back from vacation I'll be gone for a month. That
>>>>>>>> gives you a large window to push other things through with less stress
>>>>>>>> ;-)
>>>>>>>>
>>>>>>>>
>>>>>>>>> JDK-8049304 race between VM_Exit and _sync_FutileWakeups->inc()
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8049304
>>>>>>>>>
>>>>>>>>> Webrev URL:
>>>>>>>>> http://cr.openjdk.java.net/~dcubed/8049304-webrev/2-jdk9-hs-rt/
>>>>>>>>>
>>>>>>>>> The easiest way to re-review is to download the two patch files
>>>>>>>>> (round 0 and round 2) and view them in your favorite file merge tool:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~dcubed/8049304-webrev/0-jdk9-hs-rt/hotspot.patch
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~dcubed/8049304-webrev/2-jdk9-hs-rt/hotspot.patch
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Testing: Aurora Adhoc RT-SVC nightly batch (in process)
>>>>>>>>> Aurora Adhoc vm.tmtools batch (in process)
>>>>>>>>> Kim's repro sequence for JDK-8049304
>>>>>>>>> Kim's repro sequence for JDK-8129978
>>>>>>>>> JPRT -testset hotspot
>>>>>>>>>
>>>>>>>>> Changes between round 0 and round 2:
>>>>>>>>>
>>>>>>>>> - clarify a few comments
>>>>>>>>> - init _has_PerfData flag with '0' (instead of false)
>>>>>>>>> - drop unnecessary use OrderAccess::release_store() to set
>>>>>>>>> _has_PerfData to '1' (we're in a Mutex)
>>>>>>>>> - change perfMemory_exit() to only call PerfDataManager::destroy()
>>>>>>>>> when called at a safepoint and when the StatSampler is not
>>>>>>>>> running; this means when the VM is aborting, we no longer have
>>>>>>>>> a race between the original crash report and this code path.
>>>>>>>>>
>>>>>>>>> Changes between round 1 and round 2:
>>>>>>>>>
>>>>>>>>> - clarify a few comments
>>>>>>>>> - drop is_safe parameter to OM_PERFDATA_OP macro
>>>>>>>>> - init _has_PerfData flag with '0' (instead of false)
>>>>>>>>> - drop OrderAccess::fence() call before os::naked_short_sleep() call
>>>>>>>>> - drop PerfDataManager::has_PerfData_with_acquire()
>>>>>>>>> - drop unnecessary use OrderAccess::release_store() to set
>>>>>>>>> _has_PerfData to '1' (we're in a Mutex)
>>>>>>>>>
>>>>>>>>> I believe that I've addressed all comments from round 0 and
>>>>>>>>> from round 1.
>>>>>>>>>
>>>>>>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>>>>>>
>>>>>>>>> Dan
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/31/15 4:51 PM, Daniel D. Daugherty wrote:
>>>>>>>>>> Greetings,
>>>>>>>>>>
>>>>>>>>>> I've updated the "fix" for this bug based on code review comments
>>>>>>>>>> received in round 0.
>>>>>>>>>>
>>>>>>>>>> JDK-8049304 race between VM_Exit and _sync_FutileWakeups->inc()
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8049304
>>>>>>>>>>
>>>>>>>>>> Webrev URL:
>>>>>>>>>> http://cr.openjdk.java.net/~dcubed/8049304-webrev/1-jdk9-hs-rt/
>>>>>>>>>>
>>>>>>>>>> The easiest way to re-review is to download the two patch files
>>>>>>>>>> and view them in your favorite file merge tool:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~dcubed/8049304-webrev/0-jdk9-hs-rt/hotspot.patch
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~dcubed/8049304-webrev/1-jdk9-hs-rt/hotspot.patch
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Testing: Aurora Adhoc RT-SVC nightly batch (in process)
>>>>>>>>>> Aurora Adhoc vm.tmtools batch (in process)
>>>>>>>>>> Kim's repro sequence for JDK-8049304
>>>>>>>>>> Kim's repro sequence for JDK-8129978
>>>>>>>>>> JPRT -testset hotspot
>>>>>>>>>>
>>>>>>>>>> Changes between round 0 and round 1:
>>>>>>>>>>
>>>>>>>>>> - add an 'is_safe' parameter to the OM_PERFDATA_OP macro;
>>>>>>>>>> safepoint-safe callers can access _has_PerfData flag directly;
>>>>>>>>>> non-safepoint-safe callers use a load-acquire to fetch the
>>>>>>>>>> current _has_PerfData flag value
>>>>>>>>>> - change PerfDataManager::destroy() to simply set _has_PerfData
>>>>>>>>>> to zero (field is volatile) and then use a fence() to prevent
>>>>>>>>>> any reordering of operations in any direction; it's only done
>>>>>>>>>> once during VM shutdown so...
>>>>>>>>>> - change perfMemory_exit() to only call PerfDataManager::destroy()
>>>>>>>>>> when called at a safepoint and when the StatSample is not
>>>>>>>>>> running; this means when the VM is aborting, we no longer have
>>>>>>>>>> a race between the original crash report and this code path.
>>>>>>>>>>
>>>>>>>>>> I believe that I've addressed all comments from round 0.
>>>>>>>>>>
>>>>>>>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>>>>>>>
>>>>>>>>>> Dan
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 8/25/15 3:08 PM, Daniel D. Daugherty wrote:
>>>>>>>>>>> Greetings,
>>>>>>>>>>>
>>>>>>>>>>> I have a "fix" for a long standing race between JVM shutdown and the
>>>>>>>>>>> JVM statistics subsystem:
>>>>>>>>>>>
>>>>>>>>>>> JDK-8049304 race between VM_Exit and _sync_FutileWakeups->inc()
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8049304
>>>>>>>>>>>
>>>>>>>>>>> Webrev URL:
>>>>>>>>>>> http://cr.openjdk.java.net/~dcubed/8049304-webrev/0-jdk9-hs-rt/
>>>>>>>>>>>
>>>>>>>>>>> Testing: Aurora Adhoc RT-SVC nightly batch
>>>>>>>>>>> Aurora Adhoc vm.tmtools batch
>>>>>>>>>>> Kim's repro sequence for JDK-8049304
>>>>>>>>>>> Kim's repro sequence for JDK-8129978
>>>>>>>>>>> JPRT -testset hotspot
>>>>>>>>>>>
>>>>>>>>>>> This "fix":
>>>>>>>>>>>
>>>>>>>>>>> - adds a volatile flag to record whether PerfDataManager is holding
>>>>>>>>>>> data (PerfData objects)
>>>>>>>>>>> - adds PerfDataManager::has_PerfData() to return the flag
>>>>>>>>>>> - changes the Java monitor subsystem's use of PerfData to
>>>>>>>>>>> check both allocation of the monitor subsystem specific
>>>>>>>>>>> PerfData object and the new PerfDataManager::has_PerfData()
>>>>>>>>>>> return value
>>>>>>>>>>>
>>>>>>>>>>> If the global 'UsePerfData' option is false, the system works as
>>>>>>>>>>> it did before. If 'UsePerfData' is true (the default on non-embedded
>>>>>>>>>>> systems), the Java monitor subsystem will allocate a number of
>>>>>>>>>>> PerfData objects to record information. The objects will record
>>>>>>>>>>> information about Java monitor subsystem until the JVM shuts down.
>>>>>>>>>>>
>>>>>>>>>>> When the JVM starts to shutdown, the new PerfDataManager flag will
>>>>>>>>>>> change to false and the Java monitor subsystem will stop using the
>>>>>>>>>>> PerfData objects. This is the new behavior. As noted in the comments
>>>>>>>>>>> I added to the code, the race is still present; I'm just changing
>>>>>>>>>>> the order and the timing to reduce the likelihood of the crash.
>>>>>>>>>>>
>>>>>>>>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>>>>>>>>
>>>>>>>>>>> Dan
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
