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

[Daniel D. Daugherty]

On 5/27/15 2:58 PM, Karen Kinnear wrote:
> Dan,

Sorry for the delay in responding to your review. It didn't make sense
(to me) to move forward with this bucket while the regression introduced
by the "fast enter" bucket (JDK-8077392) remained in such an unknown
state. While still unresolved, much is known about JDK-8077392 and I
feel more comfortable about moving forward with the "fast notify"
bucket (JDK-8075171).


> The code looks really good.

Thanks!


> Thank you for the detailed description and the
> extensive testing.
>
> Two minor comments:
> 1. objectMonitor.cpp ~ 1706 - after the else if (policy == 2)

You didn't mention it, but there's a duplicated comment on
L1706 and L1707; I'll fix that also.


> - could you possibly move the iterator->TState = ObjectWaiter::TS_CXQ to before the if (List == NULL)/else?
>     - otherwise I think it is only set on the else

I think the existing code is correct:

1708       if (list == NULL) {
1709         iterator->_next = iterator->_prev = NULL;
1710         _EntryList = iterator;
1711       } else {
1712         iterator->TState = ObjectWaiter::TS_CXQ;
1713         for (;;) {
1714           ObjectWaiter * front = _cxq;
1715           iterator->_next = front;
1716           if (Atomic::cmpxchg_ptr(iterator, &_cxq, front) == front) {
1717             break;
1718           }
1719         }

We set the TState to TS_CXQ on L1712 because we are prepending
stuff from the _cxq on L1714-1716.
In the if-block on L1708-1710, we aren't doing anything with
_cxq so no need to change TState from its current value.


>
> 2. opto.runtime.cpp
> lines 433-435 -- did you want the comment about intrinsifying hashCode()? My memory is we aren't picking up
> those changes

I've gone back and forth about leaving that comment there or
moving it to the hashcode() bucket. We aren't picking up the
hashcode() changes as part of this JEP. I'll go back to the
original changes and see if I can remember why I put it in
this bucket; at the time, I had a good reason... :-)

Thanks for your review! And again, sorry about the delay in responding.

Dan


>
> thanks,
> Karen
>
>
> On May 18, 2015, at 4: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)