RFR(L): 8226705: [REDO] Deoptimize with handshakes

Thu Aug 29 10:01:47 UTC 2019

Hi Robbin,

Glad to see this work making it back again. Last time this patch was 
proposed, there were no guards against non-monotonic nmethod state 
transitions. Today there are (since 
https://bugs.openjdk.java.net/browse/JDK-8224674). In other words, if 
you call make_not_entrant() on an nmethod that has racingly become 
zombie, it is today impossible to resurrect that nmethod. Instead, 
make_not_entrant() will return false.

In particular, I think that is what the following code in the patch is 
guarding against (in codeCache.cpp):

1150 // Mark methods for deopt (if safe or possible).
1151 void CodeCache::mark_all_nmethods_for_deoptimization() {
1152   MutexLocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
1153   CompiledMethodIterator 
iter(CompiledMethodIterator::only_alive_and_not_unloading);
1154   while(iter.next()) {
1155     CompiledMethod* nm = iter.method();
1156     if (!nm->method()->is_method_handle_intrinsic() &&
1157         !nm->is_not_installed() &&
1158         nm->is_in_use() &&
1159         !nm->is_native_method()) {
1160       // Intrinsics and native methods are never deopted. A method 
that is
1161       // not installed yet or is not in use is not safe to deopt; the
1162       // is_in_use() check covers the not_entrant and not zombie cases.
1163       // Note: A not_entrant method can become a zombie at anytime 
if it was
1164       // made not_entrant before the previous safepoint/handshake.
1165       nm->mark_for_deoptimization();
1166     }
1167   }
1168 }

In fact, this code is also guarding against is_not_installed nmethods 
(which was introduced as a new function). If we find that an nmethod is 
bad and say it needs deoptimization, and that nmethod is just about to 
get installed, it seems to me that skipping that nmethod and not making 
it not entrant is really dangerous and will eventually lead to a crash. 
Because in ciEnv.cpp we set_code to the about to be installed nmethod, 
and it will get called without anything stopping the invalid code from 
being executed by mutators, and it was being invalidated for a reason. 
So basically we really do have to make these not entrant, or we will 
blow up.

I guess the reason you needed this check is because make_in_use() 
doesn't check what the current state is, causing monotonicity problems, 
making a potentially already not entrant nmethod become in_use, 
eventually blowing up asserts and probably the VM. Regrettably, 
make_in_use() still does that, because I had no need for changing it to 
use nmethod::try_transition in 8224674, nobody previously could find 
these nmethods. In retrospect, not changing that was a bit silly. So if 
we just change make_in_use() to use nmethod::try_transition instead, 
then you can remove the !is_not_installed check... which I argue we have 
to do. The reason we never ran in to this before is because we made the 
nmethods not entrant in a safepoint, and between making an nmethod and 
making it in_use, there is no safepoint check, so they were never 
observed in this state.

... and similarly here:

1192     if (nm->is_marked_for_deoptimization() && nm->is_in_use()) {
1193       // only_alive_and_not_unloading() can return not_entrant 
nmethods.
1194       // A not_entrant method can become a zombie at anytime if it was
1195       // made not_entrant before the previous safepoint/handshake. The
1196       // is_in_use() check covers the not_entrant and not zombie cases
1197       // that have become true after the method was marked for deopt.
1198       nm->make_not_entrant();
1199     }

... we now don't need the guard against is_in_use() here.

For what I propose to work out, we need to change nmethod::make_in_use 
to use nmethod::try_transition, and AOTCompiledMethod::make_entrant() 
needs to also return false when encountering a not_entrant nmethod, 
enforcing monotonicity, instead of asserting about that. AOT methods may 
turn not_in_use AOT methods to in_use, but not not_entrant to in_use. 
Once not_entrant, an AOT methods should remain not entrant forever.

So basically, these guards are for something that used to be racy and 
dangerous due to the lack of guards inside of the state transitions, and 
today that is completely harmless, as the proper guards are in the 
attempts to change state. I would prefer to remove these guards, as it 
is confusing to the reader that we are guarding against a problem that 
can't happen. So I think you can remove the various checks for what 
state the nmethod is in, the comments describing races, and just keep 
the checks if it's a method handle intrinsic or native method.

In deoptimize.cpp, the fetch_unroll_info_helper function has moved down, 
making it difficult to review as shows the code delta as if everything 
changed. So I can't see what actually changed there. :( Would you mind 
moving that code back so the webrev shows what changed there?

Thanks,
/Erik

On 2019-08-28 11:57, Robbin Ehn wrote:
> Hi, hotspot-dev was the intended list.
> 
> Thanks, Robbin
> 
> On 2019-08-28 09:30, Robbin Ehn wrote:
>> Hi all, please consider,
>>
>> To get away from the issues of us revoking in the handshake, but 
>> before deopt
>> the locks get re-biased before we hit deopt handler, we instead revoke 
>> in deopt
>> handler.
>> The deopt handler have a JRT_BLOCK/_END which can safepoint so we 
>> revoke after
>> that but before we start looking at the monitors, with a 
>> NoSafepointVerifier.
>>
>> Here is the previous changeset on top of jdk/jdk tip but due to merge 
>> conflicts
>> some pieces did not apply:
>> http://cr.openjdk.java.net/~rehn/8226705/v1/8226705-8221734-baseline/webrev/ 
>>
>> So this is what was reviewed.
>>
>> The rebase (merge conflict resolutions) and 8224795 bug fix :
>> http://cr.openjdk.java.net/~rehn/8226705/v1/8226705-rebase/webrev/
>>
>> Bug 8224795 fix is here:
>> http://cr.openjdk.java.net/~rehn/8226705/v1/8226705-rebase/webrev/src/hotspot/share/code/nmethod.cpp.sdiff.html 
>>
>>
>> After this we have the same functionally as the reverted change-set.
>>
>> Here is the changes for doing the revoke in deopt handler instead:
>> http://cr.openjdk.java.net/~rehn/8226705/v1/8226705-revoke-on-deopt/webrev/ 
>>
>>
>> This code was messy adding the deopt revocation in 3 places made it 
>> worse.
>> Here is a refactoring of that code. (also removed a dead method in 
>> biasedlocking):
>> http://cr.openjdk.java.net/~rehn/8226705/v1/8226705-refactor/webrev/
>>
>> And here is full:
>> http://cr.openjdk.java.net/~rehn/8226705/v1/full/webrev/
>>
>> Also a full squashed patch file:
>> http://cr.openjdk.java.net/~rehn/8226705/v1/full/full.patch
>>
>> Latest test run I did t1-t6, Linux/Windows x64 have passed, MacOSX still
>> running.
>>
>> Thanks, Robbin
>>
>>