RFR(m): 8221734: Deoptimize with handshakes

[Patricio Chilano]

Hi Robbin,

Biased locking changes look good to me.

Just a small comment based on one of the things I mentioned before. I 
can't help to think that since get_monitors_from_stack() will return 
monitors owned by the handshaked thread then the assert in 
Deoptimization::revoke_handshake():

assert(!mark->has_bias_pattern() || mark->biased_locker() == thread, 
"Can't revoke");

should be combined by the later guarantee() in 
BiasedLocking::revoke_own_locks_in_handshake() to be:

guarantee(!mark->has_bias_pattern() || (mark->biased_locker() == thread 
&& prototype_header->bias_epoch() == mark->bias_epoch()), "Can't revoke");

and that would make more evident we can remove the call to fast_revoke() 
in BiasedLocking::revoke_own_locks_in_handshake(). I know you said you 
don't want to change biased locking behavior, but I don't think doing 
that should change anything. We know the condition above should hold, 
otherwise, if the handshaked thread is not the real biaser (because of 
expired epoch) then we could hit the guarantee() in 
BiasedLocking:revoke_own_locks_in_handshake() anyways later on if some 
other thread rebiased the lock before we were able to revoke it.

Thanks!

Patricio


On 5/15/19 2:26 AM, Robbin Ehn wrote:
> Hi, please see this update.
>
> I think I got all review comments fix.
>
> Long story short, I was concerned about test coverage, so I added a 
> stress test
> using the WB, which sometimes crashed in rubbish code.
>
> There are two bugs in the methods used by WB_DeoptimizeAll.
> (Seems I'm the first user)
>
> CodeCache::mark_all_nmethods_for_deoptimization();
> When iterating the nmethods we could see the methods being create in:
> void AdapterHandlerLibrary::create_native_wrapper(const methodHandle& 
> method)
> And deopt the method when it was in use or before.
> Native wrappers are suppose to live as long as the class.
> I filtered out not_installed and native methods.
>
> Deoptimization::deoptimize_all_marked();
> The issue is that a not_entrant method can go to zombie at anytime.
> There are several ways to make a nmethod not go zombie: nmethodLocker, 
> have it
> on stack, avoid safepoint poll in some states, etc.., which is also 
> depending on
> what type of nmethod.
> The iterator only_alive_and_not_unloading returns not_entrant 
> nmethods, but we
> don't know there state prior last poll.
> in_use -> not_entrant -> #poll# -> not_entrant -> zombie
> If the iterator returns the nmethod after we passed the poll it can 
> still be
> not_entrant but go zombie.
> The problem happens when a second thread marks a method for deopt and 
> makes it
> not_entrant. Then after a poll we end-up in deoptimize_all_marked(), 
> but the
> method is not yet a zombie, so the iterator returns it, it becomes a 
> zombie thus
> pass the if check and later hit the assert.
> So there is a race between the iterator check of state and 
> if-statement check of
> state. Fixed by also filtering out zombies.
>
> If the stress test with correction of the bugs causes trouble in 
> review, I can
> do a follow-up with the stress test separately.
>
> Good news, no issues found with deopt with handshakes.
>
> This is v3:
> http://cr.openjdk.java.net/~rehn/8221734/v3/webrev/
>
> This full inc from v2 (review + stress test):
> http://cr.openjdk.java.net/~rehn/8221734/v3/inc/
>
> This inc is the review part from v2:
> http://cr.openjdk.java.net/~rehn/8221734/v3/inc_review/
>
> This inc is the additional stress test with bug fixes:
> http://cr.openjdk.java.net/~rehn/8221734/v3/inc_test/
>
> Additional biased locking change:
> The original code use same copy of markOop in revoke_and_rebias.
> The keep same behavior I now pass in that copy into fast_revoke.
>
> The stress test passes hundreds of iterations in mach5.
> Thousands stress tests locally, the issues above was reproduce-able.
> Inc changes also passes t1-5.
>
> As usual with this change-set, I'm continuously running more test.
>
> Thanks, Robbin
>
> On 2019-04-25 14:05, 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
>> 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