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

Wed Jul 8 17:52:23 UTC 2015

Karen,

Thanks for closing the loop on this piece...

Last thing is the re-review when you get the chance...

Dan

On 7/8/15 11:31 AM, Karen Kinnear wrote:
> 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)
>
>