RFR(S) Contended Locking fast notify bucket (8075171)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Jun 16 00:02:14 UTC 2015
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