RFR(m): 8221734: Deoptimize with handshakes

Wed May 8 03:06:11 UTC 2019

Hi Robbin,

On 5/7/19 10:26 AM, Robbin Ehn wrote:
> Hi Patricio,
>
> On 5/6/19 6: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().
>
> I tried to have as little changes possible. What you are saying are 
> true for
> current code also. Since often there are no locks on stack, the biggest
> over-head is walking the entire stack, twice if a lock is found. A 
> couple of
> comparison doesn't really matter here.
> I wanted the biased code to be the same as before, since this is not 
> suppose todo change anything there, but we needed to avoid triggering 
> a VM_BulkRevokeBias, so I avoided the update_heuristics.
Ok, maybe we can simplify it later then.

>> 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()?
>
> If we would failed here (hardware fault or so), you can loose your 
> lock, I'll rather have a crash in the release build, than having to 
> try debugging an application that sometimes spontaneous drops a lock, 
> so I'd like to keep a hard crash.
>
> In your suggestion we should have an guarantee, e.g:
> +  guarantee(mark->biased_locker() == THREAD &&
> +            prototype_header->bias_epoch() == mark->bias_epoch(), 
> "Revoke failed, unhandled biased lock state");
> +  ResourceMark rm;
> +  log_info(biasedlocking)("Revoking bias by walking my own stack:");
> ...
>
> Seems good?
Using guarantee() sounds good.

>> 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.
>
> Yes, ok, good.
>
>>
>> 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?
>>
>
> I looked this over, we actually always only need to revoke.
> The JavaThread will inflate, if needed, on it's way to the interpreter 
> when
> unpacking the stack. (since we are inflating the stack, we must move 
> the stack
> locks which can require an inflation)
> I renamed methods and remove early inflation.
>
> I'll send an update to RFR mail.

Thanks!

Patricio
> Thanks, Robbin
>
>>
>> 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
>>>>
>>