RFR(m): 8221734: Deoptimize with handshakes

Tue May 7 14:26:41 UTC 2019

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.

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

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