RFR(m): 8221734: Deoptimize with handshakes

[Daniel D. Daugherty]

On 4/25/19 8:05 AM, Robbin Ehn wrote:
> Hi all, please review.
>
> Let's deopt with handshakes.
> Removed VM op Deoptimize, instead we handshake.
> Locks needs to be inflate since we are not in a safepoint.
>
> Goes on top of:
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-April/033491.html 
>
>
> Code:
> http://cr.openjdk.java.net/~rehn/8221734/v1/webrev/index.html

Robbin, you'll have to merge with Coleen's recent MutexLocker fix (8222518).

src/hotspot/share/aot/aotCodeHeap.cpp
     No comments.

src/hotspot/share/aot/aotCompiledMethod.cpp
     L163: bool AOTCompiledMethod::make_not_entrant_helper(int new_state) {
     L207: bool AOTCompiledMethod::make_entrant() {
         So the Compiler team is on board with switching from the
         Patching_lock to the CompiledMethod_lock?

src/hotspot/share/code/codeCache.cpp
     No comments.

src/hotspot/share/code/nmethod.cpp
     L1180: bool nmethod::make_not_entrant_or_zombie(int state) {
     L2853: void nmethod::clear_jvmci_installed_code() {
     L2861: void nmethod::clear_speculation_log() {
     L2869: void nmethod::maybe_invalidate_installed_code() {
     L2904: void nmethod::invalidate_installed_code(Handle 
installedCode, TRAPS) {
         So the Compiler team is on board with switching from the
         Patching_lock to the CompiledMethod_lock?

src/hotspot/share/code/nmethod.hpp
     No comments.

src/hotspot/share/gc/z/zBarrierSetNMethod.cpp
     No comments (copyright year needs update).

src/hotspot/share/gc/z/zNMethod.cpp
     No comments (copyright year needs update).

src/hotspot/share/oops/markOop.hpp
     L180:   bool biased_locker_is(JavaThread* thread) const {
         Is this for a different project?

     L181:     if (!has_bias_pattern()) {
     L182:       return false;
     L183:     }
     L184:     // If current thread is not the owner it can be unbiased 
at anytime.
     L185:     JavaThread* jt = (JavaThread*) ((intptr_t) 
(mask_bits(value(), ~(biased_lock_mask_in_place | age_mask_in_place | 
epoch_mask_in_place))));
         So you don't want to use this:

           JavaThread* biased_locker() const {
             assert(has_bias_pattern(), "should not call this otherwise");
             return (JavaThread*) ((intptr_t) (mask_bits(value(), 
~(biased_lock_mask_in_place | age_mask_in_place | epoch_mask_in_place))));
           }

         because of the assert().

         I think biased_locker() and biased_locker_is() both need to work
         with a copy of the markOop so that it can't change dynamically.
         Something like:

           JavaThread* biased_locker() const {
             markOop copy = this;
             assert(copy.has_bias_pattern(), "should not call this 
otherwise");
             return (JavaThread*) ((intptr_t) (copy.mask_bits(value(), 
~(biased_lock_mask_in_place | age_mask_in_place | epoch_mask_in_place))));
           }

           bool biased_locker_is(JavaThread* thread) const {
             markOop copy = this;
             if (!copy.has_bias_pattern()) {
               return false;
             }
             return copy.biased_locker();
           }

src/hotspot/share/oops/method.cpp
     old L104:   clear_code(false /* don't need a lock */); // 
from_c/from_i get set to c2i/i2i
         Is the comment after '//' not useful?

     L946:     MutexLockerEx ml(CompiledMethod_lock->owned_by_self() ? 
NULL : CompiledMethod_lock, Mutex::_no_safepoint_check_flag);
         So the Compiler team is on board with switching from the
         Patching_lock to the CompiledMethod_lock?

src/hotspot/share/oops/method.hpp
     No comments.

src/hotspot/share/prims/jvmtiEventController.cpp
     No comments (copyright year needs update).

src/hotspot/share/prims/methodHandles.cpp
     No comments.

src/hotspot/share/prims/whitebox.cpp
     No comments.

src/hotspot/share/runtime/biasedLocking.cpp
src/hotspot/share/runtime/biasedLocking.hpp
     Hmmm... More Biased Locking changes. I didn't take a close
     look at these.

src/hotspot/share/runtime/deoptimization.cpp
     No comments (but some overlap with Biased Locking, ouch)

src/hotspot/share/runtime/deoptimization.hpp
     No comments.

src/hotspot/share/runtime/mutex.hpp
     old L65:        special        = tty            +   1,
     new L65:        special        = tty            +   2,
         Why?

src/hotspot/share/runtime/mutexLocker.cpp
     No comments.

src/hotspot/share/runtime/mutexLocker.hpp
     L34: extern Mutex*   Patching_lock;                   // a lock 
used to guard code patching of compiled code
     L35: extern Mutex*   CompiledMethod_lock;
         A comment is traditional here...

src/hotspot/share/runtime/synchronizer.cpp
     old L1317:          !SafepointSynchronize::is_at_safepoint(), 
"invariant");
     new L1317:          !Universe::heap()->is_gc_active(), "invariant");
         Why?

     L1446:         ResourceMark rm;
     L1496:       ResourceMark rm;
         Why drop 'Self'? That makes the ResourceMark more expensive..

src/hotspot/share/runtime/thread.cpp
     No comments.

src/hotspot/share/runtime/thread.hpp
     No comments.

src/hotspot/share/runtime/vmOperations.cpp
     No comments.

src/hotspot/share/runtime/vmOperations.hpp
     No comments.

src/hotspot/share/services/dtraceAttacher.cpp
     No comments (copyright year needs update).


I don't think I've found anything that's "must fix". Please let me know
if I should re-review:

     src/hotspot/share/runtime/biasedLocking.cpp
     src/hotspot/share/runtime/biasedLocking.hpp
     src/hotspot/share/runtime/deoptimization.cpp

because the Biased Locking changes are critical for this project.

Dan


> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8221734
>
> Passes t1-7 and multiple t1-5 runs.
>
> A few startup benchmark see a small speedup.
>
> Thanks, Robbin