RFR(S) Contended Locking fast notify bucket (8075171)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Jul 9 13:23:18 UTC 2015
Thanks for closing the loop on this one.
Dan
On 7/9/15 7:20 AM, Karen Kinnear wrote:
> Dan,
>
> ship it!
>
> thanks,
> Karen
>
> On Jul 9, 2015, at 2:40 AM, David Holmes wrote:
>
>> On 9/07/2015 8:10 AM, Daniel D. Daugherty wrote:
>>> On 7/8/15 12:06 PM, Daniel D. Daugherty wrote:
>>>> On 7/8/15 11:53 AM, Karen Kinnear wrote:
>>>>> Dan,
>>>>>
>>>>> Looks good.
>>>>>
>>>>> Only comment is -
>>>>>
>>>>> In ObjectMonitor::INotify - lines 1668-1671
>>>>> the default value of policy is "2"
>>>>> -- which basically is "b" not "a" unless the EntryList is empty.
>>>> Nice catch! For folks on the alias that don't have this code
>>>> tattooed on the inside of their eyelids. This is the relevant code:
>>>>
>>>> src/share/vm/runtime/objectMonitor.cpp:
>>>>
>>>> 136 static int Knob_MoveNotifyee = 2; // notify() -
>>>> disposition of notifyee
>>>>
>>>> <snip>
>>>>
>>>> 1659 void ObjectMonitor::INotify(Thread * Self) {
>>>> 1660 const int policy = Knob_MoveNotifyee;
>>>>
>>>> <snip>
>>>>
>>>> 1668 // Disposition - what might we do with iterator ?
>>>> 1669 // a. add it directly to the EntryList - either tail or head.
>>>> 1670 // b. push it onto the front of the _cxq.
>>>> 1671 // For now we use (a).
>>>>
>>>> <snip>
>>>>
>>>> 1710 } else if (policy == 2) { // prepend to cxq
>>>>
>>>> I will update the comment.
>>> Don't think this update is worth a new webrev so here
>>> are the diffs instead:
>>>
>>> $ diff src/share/vm/runtime/objectMonitor.cpp{.cr1,}
>>> 1669,1671c1669,1672
>>> < // a. add it directly to the EntryList - either tail or head.
>>> < // b. push it onto the front of the _cxq.
>>> < // For now we use (a).
>>> ---
>>>> // a. add it directly to the EntryList - either tail (policy == 1)
>>>> // or head (policy == 0).
>>>> // b. push it onto the front of the _cxq (policy == 2).
>>>> // For now we use (b).
>>> Thanks again for the catch!
>> Yep good catch!
>>
>> David
>>
>>> Dan
>>>
>>>
>>>> Dan
>>>>
>>>>
>>>>> thanks,
>>>>> Karen
>>>>>
>>>>> On Jul 3, 2015, at 8:11 PM, Daniel D. Daugherty wrote:
>>>>>
>>>>>> Greetings,
>>>>>>
>>>>>> I've updated the patch for Contended Locking fast notify bucket
>>>>>> based on David H's and Karen's comments.
>>>>>>
>>>>>> This work is being tracked by the following bug ID:
>>>>>>
>>>>>> JDK-8075171 Contended Locking fast notify bucket
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8075171
>>>>>>
>>>>>> Here is the webrev URL:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/8075171-webrev/1-jdk9-hs-rt/
>>>>>>
>>>>>> Here is the JEP link:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8046133
>>>>>>
>>>>>> Testing:
>>>>>>
>>>>>> - RBT vm.quick batches (in process)
>>>>>> - JPRT test jobs
>>>>>> - MonitorWaitNotifyStresser micro-benchmark (in process)
>>>>>> - CallTimerGrid stress testing (in process)
>>>>>>
>>>>>>
>>>>>> The changes in this round are in only four of the files:
>>>>>>
>>>>>> src/share/vm/opto/runtime.cpp
>>>>>>
>>>>>> - deleted comment about hashCode() that belongs in a different bucket
>>>>>>
>>>>>> src/share/vm/runtime/objectMonitor.cpp
>>>>>>
>>>>>> - add MAX_RECHECK_INTERVAL define for use in ObjectMonitor::EnterI()
>>>>>> - cleanup recheck interval code
>>>>>> - ObjectMonitor::INotify() return type is now "void"
>>>>>> - delete duplicate comments, clarify/rewrite some comments
>>>>>>
>>>>>> src/share/vm/runtime/objectMonitor.hpp
>>>>>>
>>>>>> - ObjectMonitor::INotify() return type is now "void"
>>>>>>
>>>>>> src/share/vm/runtime/synchronizer.cpp
>>>>>>
>>>>>> - clarify/rewrite comments, add additional comments
>>>>>> - cleanup variables in ObjectSynchronizer::quick_notify
>>>>>> - refactor loop to be more clear
>>>>>> - delete unused enum MaximumRecheckInterval
>>>>>>
>>>>>> The easiest way to review the delta is to download the two patch
>>>>>> files and compare them in something like jfilemerge:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/8075171-webrev/0-jdk9-hs-rt/hotspot.patch
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dcubed/8075171-webrev/1-jdk9-hs-rt/hotspot.patch
>>>>>>
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>> On 5/18/15 2:07 PM, Daniel D. Daugherty wrote:
>>>>>>> Greetings,
>>>>>>>
>>>>>>> I have the Contended Locking fast notify bucket ready for review.
>>>>>>>
>>>>>>> The code changes in this bucket are the final piece in the triad
>>>>>>> of {fast-enter, fast-exit, fast-notify} changes. There are no
>>>>>>> macro assembler changes in this bucket (yeah!), but there are
>>>>>>> code review cleanups motivated by comments on other buckets, e.g.,
>>>>>>> changing variables like 'Tally' -> 'tally'.
>>>>>>>
>>>>>>> This work is being tracked by the following bug ID:
>>>>>>>
>>>>>>> JDK-8075171 Contended Locking fast notify bucket
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8075171
>>>>>>>
>>>>>>> Here is the webrev URL:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dcubed/8075171-webrev/0-jdk9-hs-rt/
>>>>>>>
>>>>>>> Here is the JEP link:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8046133
>>>>>>>
>>>>>>> Testing:
>>>>>>>
>>>>>>> - Aurora Adhoc RT/SVC baseline batch
>>>>>>> - JPRT test jobs
>>>>>>> - MonitorWaitNotifyStresser micro-benchmark (in process)
>>>>>>> - CallTimerGrid stress testing (in process)
>>>>>>> - Aurora performance testing:
>>>>>>> - out of the box for the "promotion" and 32-bit server configs
>>>>>>> - heavy weight monitors for the "promotion" and 32-bit server
>>>>>>> configs
>>>>>>> (-XX:-UseBiasedLocking -XX:+UseHeavyMonitors)
>>>>>>> (in process)
>>>>>>>
>>>>>>> The hang somehow introduced by the "fast enter" bucket is still
>>>>>>> unresolved, but progress is being made. That work is being tracked
>>>>>>> by the following bug:
>>>>>>>
>>>>>>> JDK-8077392 Stream fork/join tasks occasionally fail to complete
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8077392
>>>>>>>
>>>>>>> You'll see a change that re-enables the "fast enter" bucket in this
>>>>>>> webrev, but that change will not be pushed when we're done with the
>>>>>>> review of this bucket unless JDK-8077392 is resolved.
>>>>>>>
>>>>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>>>>
>>>>>>> Gory details about the changes are below...
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>> 8075171 summary of changes:
>>>>>>>
>>>>>>> src/share/vm/classfile/vmSymbols.hpp
>>>>>>> - Add do_intrinsic() entries for _notify and _notifyAll
>>>>>>>
>>>>>>> src/share/vm/opto/library_call.cpp
>>>>>>> - Add optional inline_notify() that is controlled by new
>>>>>>> '-XX:[+-]InlineNotify' option
>>>>>>>
>>>>>>> src/share/vm/opto/runtime.cpp
>>>>>>> src/share/vm/opto/runtime.hpp
>>>>>>> - Add OptoRuntime::monitor_notify_C() and
>>>>>>> OptoRuntime::monitor_notifyAll_C() functions to support
>>>>>>> the new "fast notify" code paths
>>>>>>> - Add new OptoRuntime::monitor_notify_Type() to support
>>>>>>> the above two new functions
>>>>>>>
>>>>>>> src/share/vm/runtime/globals.hpp
>>>>>>> - Add '-XX:[+-]InlineNotify' option; default option value is
>>>>>>> enabled (true)
>>>>>>>
>>>>>>> src/share/vm/runtime/objectMonitor.cpp
>>>>>>> src/share/vm/runtime/objectMonitor.hpp
>>>>>>> - Refactor ObjectMonitor::notify() and ObjectMonitor::notifyAll()
>>>>>>> into wrappers that call the new ObjectMonitor::INotify()
>>>>>>> - Add new ObjectMonitor::INotify() to reduce code duplication
>>>>>>> between "notify" and "notifyAll" code paths
>>>>>>>
>>>>>>> src/share/vm/runtime/sharedRuntime.cpp
>>>>>>> - Temporarily re-enable the "fast enter" optimization in order
>>>>>>> to do performance testing. JDK-8077392 is still unresolved,
>>>>>>> but progress is being made.
>>>>>>>
>>>>>>> src/share/vm/runtime/synchronizer.cpp
>>>>>>> src/share/vm/runtime/synchronizer.hpp
>>>>>>> - Add ObjectSynchronizer::quick_notify() that implements the
>>>>>>> new "fast notify" code paths
>>>>>>> - The new code handles a couple of special cases where the
>>>>>>> "notify" can be done without the heavy weight slow-path
>>>>>>> (thread state transitions and other baggage)
>>>>>>>
>>>>>>>
>>>>
>>>>
>
>
More information about the hotspot-runtime-dev
mailing list