RFR(S) Contended Locking fast notify bucket (8075171)

Wed Jul 8 22:10:03 UTC 2015

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!

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