RFR(S) Contended Locking fast notify bucket (8075171)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Jul 8 17:25:47 UTC 2015
Karen,
No new RFEs filed yet as part of this bucket (8075171).
I think the migration of ideas and what-ifs needs to happen
under another bug fix. We also need to figure out where to
move this stuff. The wikis you mention below are Oracle
internal so I don't think those are a good place to land
content that is currently in OpenJDK.
Dan
On 7/8/15 10:58 AM, Karen Kinnear wrote:
> Dan,
>
> I don't know if you created rfes or not for the potential ideas.
>
> What I did when I cleaned this up earlier was move them to a wiki page
> http://j2se.us.oracle.com/web/bin/view/HotspotRuntime/SyncFutures
>
> So we could create rfes when we thought we might be actually adopting the ideas.
>
> This is linked off of the runtime brainstorming page in the contended locking section:
> https://wiki.se.oracle.com/display/JPG/JVM+Runtime+Brainstorming
>
> Feel free to modify either of those pages with new ideas.
>
> hope this helps,
> Karen
>
> On Jun 15, 2015, at 8:02 PM, Daniel D. Daugherty wrote:
>
>> On 5/26/15 12:50 AM, David Holmes wrote:
>>> Hi Dan,
>>>
>>> Sorry for the delay on this. And thanks for the detailed explanation of the changes.
>> 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).
>>
>>
>>> Overall looks good only a couple of nits. :)
>> Thanks!
>>
>>
>>> See below ...
>> As usual, replies are embedded below.
>>
>>
>>> On 19/05/2015 6:07 AM, 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
>>> Ok
>>>
>>>> src/share/vm/opto/library_call.cpp
>>>> - Add optional inline_notify() that is controlled by new
>>>> '-XX:[+-]InlineNotify' option
>>> Ok
>>>
>>>> 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
>>> Ok
>>>
>>>> src/share/vm/runtime/globals.hpp
>>>> - Add '-XX:[+-]InlineNotify' option; default option value is
>>>> enabled (true)
>>> Ok
>>>
>>>> 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
>>> Why does INotify return int ?
>> I'm going to guess that this is a left-over from an earlier version of
>> the refactor of ObjectMonitor::notify() and ObjectMonitor::notifyAll().
>> I will change it to return 'void'.
>>
>>
>>> The soliloquy starting:
>>>
>>> 1762 // Consider: a non-uncommon synchronization bug is to use notify() when notifyAll()
>>>
>>> seems somewhat out of context without some option to turn notify into notifyAll,
>> The prefix of "Consider:" in a comment marks an idea that we should
>> consider implementing. We're thinking about moving these ideas into
>> RFE's in order to improve the readability of the code as it stands
>> today.
>>
>>
>>> or apply the MinimumWait.
>> The '-XX:SyncFlags=1' option causes all park() operations to use a small
>> timer value so it is a good way to check for lost unpark() operations.
>> 'MinimumWait' doesn't exist as anything other than comments. I suspect
>> that it was superseded by RecheckInterval and MaximumRecheckInterval.
>> I'll look at fixing those comments.
>>
>>
>>> Further a lost wakeup need not imply incorrect use of notify rather than notifyAll so there are really two different debugging techniques being described.
>> I don't think the new comment block is trying to say that the only
>> source for a "lost wakeup" bug is due to the use of notify() when
>> notifyAll() is more appropriate. I'll see if I can tweak the wording
>> a little to make it clear that the use of notify() when notifyAll()
>> is more appropriate is just an example.
>>
>>
>>>> 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.
>>> Ok
>>>
>>>> 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 comment block starting:
>>>
>>> 150 // An interesting optimization is to have the JIT recognize the following
>>>
>>> seems to be an other soliloquy. While interesting I initially thought it was describing what quick_notify was doing - which it isn't. Maybe that commentary belongs somewhere in the compiler code where the intrinsification is being done?
>> L150-L155 should have been marked with a "Consider:" tag. This
>> one screams to be moved into an RFE.
>>
>>
>>> The 'All' parameter to quick_notify should be 'all'.
>> Yes and the 'Self' parameter should be 'self'. I thought I had
>> fixed those for all new functions. Guess not. :-(
>>
>>
>>> The comment block at lines 176 - 180 should be moved prior to line 171, because the check at 171 is the "not biased" case, and the check at 174 is the "owned by caller" case.
>> 165 if (mark->has_locker() && Self->is_lock_owned((address)mark->locker())) {
>>
>> The if-block starting at line 165 is the stack locked case.
>>
>> 171 if (mark->has_monitor()) {
>>
>> The if-block starting at L171 is the inflated monitor case.
>>
>> 174 if (mon->owner() != Self) return false;
>>
>> The if-statement on L174 is the ownership sanity check for the
>> notify/notifyAll operation. This shouldn't happen because a
>> thread that doesn't own the Java Monitor should not call notify()
>> or notifyAll() on it, but we take the slow path in order to
>> provide a better failure mode.
>>
>> The biased locking case actually hits here:
>>
>> 201 return false; // revert to slow-path
>>
>>
>> I think I need to rip out much of the comment from L176-L180 and
>> maybe move some of it elsewhere.
>>
>>
>>> This loop:
>>>
>>> 188 for (;;) {
>>> 189 if (mon->first_waiter() == NULL) break;
>>> 190 mon->INotify(Self);
>>> 191 ++tally;
>>> 192 if (!All) break;
>>> 193 }
>>>
>>> would be a bit cleaner to me if written:
>>>
>>> do {
>>> mon->INotify(Self);
>>> ++tally;
>>> } while (mon->first_waiter() != NULL && All);
>> Since that loop is protected by an initial first_waiter() check:
>>
>> 181 if (mon->first_waiter() != NULL) {
>>
>> I concur that your loop is better. I'll fix it.
>>
>> Thanks for your review! And again, sorry about the delay in responding.
>>
>> Dan
>>
>>
>>> Thanks,
>>> David
>>>
>>>> - 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