RFR(m): 8221734: Deoptimize with handshakes

[Patricio Chilano]

Sorry, in fourth paragraph I meant: 
s/BiasedLocking::revoke_and_rebias()/revoke_bias()

Thanks,
Patricio

On 5/6/19 12:10 PM, Patricio Chilano wrote:
> Hi Robbin,
>
> I'm going to just review the biased locking part since I'm not really 
> familiar with the rest of the code.
>
> In BiasedLocking::revoke_and_rebias_in_handshake(), why do you need to 
> execute fast_revoke(obj, false)? If these are objects locked by the 
> JavaThread you are handshaking then it seems they should be normal 
> locks (no bias pattern) or the condition (mark->biased_locker() == 
> THREAD && prototype_header->bias_epoch() == mark->bias_epoch()) you 
> are testing for later should hold. Then that would save the extra 
> comparisons in fast_revoke().
>
> Also instead of placing the condition (mark->biased_locker() == THREAD 
> && prototype_header->bias_epoch() == mark->bias_epoch()) inside an 
> if() and then later use a ShouldNotReachHere(), wouldn't it be better 
> to make that an assertion, place that code outside the if() and remove 
> the ShouldNotReachHere()?
>
> For the execution of revoke_bias() inside 
> BiasedLocking::revoke_and_rebias_in_handshake() you could use a 
> shorter version of BiasedLocking::revoke_and_rebias() that avoids the 
> extra comparisons made for the general case and just starts at the 
> walking the stack part, but I'm actually doing that for 8191890 so I 
> can merge that with my patch.
>
> In deoptimization.cpp you have methods inflate_monitors() and 
> inflate_monitors_handshake(), but in inflate_monitors() you are not 
> inflating the monitors, you just revoke the ones that have bias. You 
> mentioned in your first email that we need to inflate if we are not at 
> a safepoint, why is that? Since revocation seems to be the common 
> factor between those methods, maybe s/inflate/revoke is a better name?
>
>
> Thanks!
> Patricio
>
>
> On 5/6/19 4:42 AM, Robbin Ehn wrote:
>> Hi Dan,
>>
>>> src/hotspot/share/runtime/biasedLocking.cpp
>>>      nit - Please update copyright year for this file.
>>>
>>
>> Updated in 8220724.
>>
>>>      Nice refactoring into more readable chunks! I'm assuming that
>>>      Patricio is also reviewing these changes...
>>
>> Great, good!
>>
>>> src/hotspot/share/runtime/deoptimization.cpp
>>>      L778:  bool _in_handshake;
>>>          nit - needs one more space of indent.
>>
>> Fixed.
>>
>>>
>>>      Nice refactoring while adding in the handshake support.
>>
>> Great!
>>
>>>
>>> src/hotspot/share/runtime/deoptimization.hpp
>>>      L147:  public:
>>>      L148:
>>>      L149:   // Deoptimizes a frame lazily. nmethod gets patched 
>>> deopt happens on return to the frame
>>>      L163:   static void fix_monitors(JavaThread* thread, frame fr, 
>>> RegisterMap* map)
>>>          Style nit: I would put the blank line on L148 above L147.
>>
>> Fixed.
>>
>>>
>>>      L164:     { inflate_monitors(thread, fr, map); }
>>>          Style nit: Should be:
>>>
>>>              static void fix_monitors(JavaThread* thread, frame fr, 
>>> RegisterMap* map) {
>>>                inflate_monitors(thread, fr, map);
>>>              }
>>
>> Fixed.
>>
>>> src/hotspot/share/runtime/mutexLocker.cpp
>>>      No comments. (So OsrList_lock is now 'special-1' instead of 
>>> 'leaf'.
>>>      I presume the Compiler team is okay with that...
>>
>> Since need we hold CodeCache_lock while iterating nmethods, all locks 
>> that might be taken needed to be pushed down under CodeCache_lock.
>> So I hope they are okay with that.
>>
>>> Thumbs up!  I don't need to see a webrev if you fix the nits...
>>
>> Thanks Dan! Fixed!
>>
>> I did t6-7 over the weekend, no issues found.
>>
>> /Robbin
>>
>>>
>>> Dan
>>>
>>>
>>>>
>>>> # Note
>>>> http://cr.openjdk.java.net/~rehn/8221734/v2/inc/webrev/src/hotspot/share/runtime/biasedLocking.cpp.sdiff.html 
>>>> line 630
>>>> This is revert to the original, I accidental had left in a 
>>>> temporary test change, as you can see here in full diff:
>>>> http://cr.openjdk.java.net/~rehn/8221734/v2/webrev/src/hotspot/share/runtime/biasedLocking.cpp.sdiff.html 
>>>>
>>>>
>>>> I think I manage to address all review comments.
>>>>
>>>> Dean can you please cast an extra eye on:
>>>> http://cr.openjdk.java.net/~rehn/8221734/v2/inc/webrev/src/hotspot/share/oops/method.cpp.sdiff.html 
>>>>
>>>> This OR should be correct.
>>>>
>>>> Dan please do the same on the biased locking changes.
>>>>
>>>> I left out the merge with MutexLocker changes, since it was not 
>>>> interesting.
>>>> There were some conflicts with JVMCI changes, so incremental 
>>>> contains some parts of that merge.
>>>>
>>>> Passes t1-5 and local testing.
>>>> I'll continue with some additional testing.
>>>>
>>>> Thanks, Robbin
>>>>
>>>> On 4/25/19 2:05 PM, 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
>>>
>