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

[David Holmes]

Hi Dan,

Sorry for the delay on this. And thanks for the detailed explanation of 
the changes.

Overall looks good only a couple of nits. :)

See 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 ?

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, or apply the MinimumWait. Further a lost wakeup need not 
imply incorrect use of notify rather than notifyAll so there are really 
two different debugging techniques being described.

> 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?

The 'All' parameter to quick_notify should be 'all'.

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.

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);

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)