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

[Karen Kinnear]

Dan,

On Jun 15, 2015, at 8:02 PM, Daniel D. Daugherty wrote:

> 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
My bad - the TState is set to TS_ENTER above if policy != 4 - I missed that.

thanks,
Karen
> 
> 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)
>