RFR(m): 8221734: Deoptimize with handshakes

[David Holmes]

Hi Robbin,

On 15/05/2019 4:26 pm, Robbin Ehn wrote:
> Hi, please see this update.
> 
> I think I got all review comments fix.

The changes related to my comments all seem fine - thanks.

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

I'm not concerned about combining these.

One nit in the test:

59             Thread.currentThread().sleep(10);

should just be Thread.sleep(10) as its not an instance method.

Thanks,
David
-----

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