RFR(S) Contended Locking fast notify bucket (8075171)
David Holmes
david.holmes at oracle.com
Thu Jul 9 06:40:00 UTC 2015
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