RFR(m): 8221734: Deoptimize with handshakes

Tue May 7 02:49:05 UTC 2019

Hi Robbin,

I took a look at this and it is a lot more complex than I had expected. 
There are some interconnections that I'm not understanding here. From 
existing code why does deopt need to revoke biases? I don't see how 
deopt changes anything in respect to monitor ownwership. Then in the new 
code why do you have to inflate monitors to do deopt? If this is truly 
new behaviour and I didn't just miss where this happens in existing 
code, then how does this impact the number of ObjectMonitors in 
existence and the monitor deflation process?

More comments below ...

On 3/05/2019 8:31 pm, Robbin Ehn wrote:
> Hi, please see this update:
> 
> Inc:
> http://cr.openjdk.java.net/~rehn/8221734/v2/inc/webrev/index.html
> Full:
> http://cr.openjdk.java.net/~rehn/8221734/v2/webrev/

src/hotspot/share/code/codeCache.cpp

This comment block no longer applies give there is no safepoint op now:

1190   // CodeCache can only be updated by a thread_in_VM and they will 
all be
1191   // stopped during the safepoint so CodeCache will be safe to 
update without
1192   // holding the CodeCache_lock.

The same comment block here:

1208   // CodeCache can only be updated by a thread_in_VM and they will 
all be
1209   // stopped dring the safepoint so CodeCache will be safe to 
update without
1210   // holding the CodeCache_lock.

is already incorrect if not actually at a safepoint. It makes the 
relationship between the Compile_lock, CodeCache_lock and being at a 
safepoint rather confusing.

---

src/hotspot/share/code/nmethod.cpp

The comment here:

1119   // If _method is already NULL the Method* is about to be unloaded,
1120   // so we don't have to break the cycle. Note that it is possible to
1121   // have the Method* live here, in case we unload the nmethod because
1122   // it is pointing to some oop (other than the Method*) being 
unloaded.

no longer fits the code given we no longer skip the _method==NULL case. 
Further, the trailing comment here:

1123   Method::unlink_code(_method, this); // Break a cycle

seems unnecessary given we preceded this with 4 lines of commentary already!

Again this comment block:

1205 void nmethod::unlink_from_method() {
1206   // We need to check if both the _code and 
_from_compiled_code_entry_point
1207   // refer to this nmethod because there is a race in setting these 
two fields
1208   // in Method* as seen in bugid 4947125.
1209   // If the vep() points to the zombie nmethod, the memory for the 
nmethod
1210   // could be flushed and the compiler and vtable stubs could still 
call
1211   // through it.
1212   Method::unlink_code(method(), this);

seems meaningless with the code change you've applied.

---

src/hotspot/share/code/nmethod.hpp

!   void unlink_from_method();

Now this doesn't take the acquire_lock parameter it would be useful to 
document what the locking expectations are: must this be called with a 
given lock held, or will it always acquire a given lock if needed?

---

src/hotspot/share/oops/method.cpp

+ void Method::unlink_code(Method *method, CompiledMethod *compare) {
+ void Method::unlink_code(Method *method) {

I'm not sure making these methods static just so the NULL check can be 
internalized is the best way to deal with this. Now you can't tell when 
NULL is expected and when it is an error. IMHO it is better to keep 
these as instance methods with a NULL check at the callsite if needed 
(or an assert if NULL is not expected).

---

  src/hotspot/share/runtime/biasedLocking.cpp
  src/hotspot/share/runtime/biasedLocking.hpp

This seems like it would be better done after we have switched 
biased-locking revocation to use handshakes instead of safepoints. 
Otherwise we seem to be doing a partial conversion as a side-effect of 
this bug and it's far from obvious that it is complete/correct.

---

src/hotspot/share/runtime/synchronizer.cpp

These changes have me worried:

    assert(Universe::verify_in_progress() ||
!          !SafepointSynchronize::is_at_safepoint(), "invariant");

becomes:

    assert(Universe::verify_in_progress() ||
!          !Universe::heap()->is_gc_active(), "invariant");

I don't see an immediate equivalence between not being at a safepoint 
and not having a GC active. If I'm not at a safepoint now and the code 
following doesn't do a safepoint check then we remain outside of a 
safepoint. What is the same reasoning for a GC being active?

!         ResourceMark rm(Self);
!         ResourceMark rm;

Are you suggesting the current thread is not Self? If that is the case 
then there should be numerous asserts earlier on to ensure we can't 
follow any code paths that expect that Self is the current thread! But 
I'm concerned that we've introduced a new way for a third-party thread 
to introduce monitor inflation almost independent of the threads using 
the monitor.

Thanks,
David
-----

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