RFR(m): 8221734: Deoptimize with handshakes

[Patricio Chilano]

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