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

[Karen Kinnear]

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.

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