RFR(m): 8221734: Deoptimize with handshakes
Robbin Ehn
robbin.ehn at oracle.com
Tue May 21 09:39:31 UTC 2019
Hi Dan,
On 2019-05-21 00:33, Daniel D. Daugherty wrote:
> src/hotspot/share/code/codeCache.cpp
> L1145: // Deoptimize all(most) methods
> Yea... that comment and the name of the function need help even
> before your changes. Sigh... Maybe:
>
> // Mark methods for deopt (if safe or possible).
Changed.
>
> L1151: // Not installed are unsafe to mark for deopt, normally never
> deopted.
> L1152: // A not_entrant method may become a zombie at any time,
> L1153: // since we don't know on which side of last safepoint it became
> not_entrant
> L1154: // (state must be in_use).
> L1155: // Native method are unsafe to mark for deopt, normally never
> deopted.
> L1156: if (!nm->method()->is_method_handle_intrinsic() &&
> L1157: !nm->is_not_installed() &&
> L1158: nm->is_in_use() &&
> L1159: !nm->is_native_method()) {
> L1160: nm->mark_for_deoptimization();
> Please consider replacing L1151-5 with the following comment after
> L1159. I prefer the comment for an if-statement to be inside the
> if-statement (but this is Compiler team code so they may have a
> different preference).
>
> // Intrinsics and native methods are never deopted. A method
> that is
> // not installed yet or is not in use is not safe to deopt; the
> // is_in_use() check covers the not_entrant and not zombie cases.
> // Note: A not_entrant method can become a zombie at anytime
> if it was
> // made not_entrant before the previous safepoint/handshake.
Changed.
>
> L1187: // only_alive_and_not_unloading returns not_entrant nmethods.
> L1188: // A not_entrant can become a zombie at anytime,
> L1189: // if it was made not_entrant before previous safepoint/handshake.
> L1190: // We check that it is not not_entrant and not zombie,
> L1191: // by checking is_in_use().
> L1192: if (nm->is_marked_for_deoptimization() && nm->is_in_use()) {
> L1193: nm->make_not_entrant();
> Please consider replacing L1187-91 with the following comment after
> L1192 (also indented):
> // only_alive_and_not_unloading() can return not_entrant
> nmethods.
> // A not_entrant method can become a zombie at anytime if it was
> // made not_entrant before the previous safepoint/handshake. The
> // is_in_use() check covers the not_entrant and not zombie cases
> // that have become true after the method was marked for deopt.
Changed.
>
> src/hotspot/share/code/compiledMethod.hpp
> No comments.
>
> src/hotspot/share/code/nmethod.cpp
> No comments.
>
> src/hotspot/share/oops/method.cpp
> L957: // We need to check if both the _code and
> _from_compiled_code_entry_point
> L958: // refer to this nmethod because there is a race in setting these
> two fields
> L959: // in Method* as seen in bugid 4947125.
> L960: // If the vep() points to the zombie nmethod, the memory for the
> nmethod
> L961: // could be flushed and the compiler and vtable stubs could still call
> L962: // through it.
> L963: if (code() == compare ||
> L964: from_compiled_entry() == compare->verified_entry_point()) {
> Now that you've moved the comment closer to the code, I can see a
> disconnect between the comment and the code. The comment:
>
> // ... check if both the _code and _from_compiled_code_entry_point
> // refer to this nmethod
>
> The code:
>
> code() == compare ||
> from_compiled_entry() == compare->verified_entry_point()
>
> So the comment is "both" "and" and the the code is "||". One of
> these is not right.
Comment is wrong, it's or, changed:
// We need to check if either the _code or _from_compiled_code_entry_point
This is the original:
// We need to check if both the _code and _from_compiled_code_entry_point
...
if (method() != NULL && (method()->code() == this ||
method()->from_compiled_entry() ==
verified_entry_point())) {
>
> src/hotspot/share/oops/method.hpp
> No comments.
>
> src/hotspot/share/prims/whitebox.cpp
> No comments.
>
> src/hotspot/share/runtime/biasedLocking.cpp
> No comments.
>
> src/hotspot/share/runtime/biasedLocking.hpp
> No comments.
>
> src/hotspot/share/runtime/deoptimization.cpp
> L1297: void Deoptimization::revoke_safepoint(JavaThread* thread, frame fr,
> RegisterMap* map) {
> Perhaps revoke_using_safepoint().
Changed.
>
> L1311: void Deoptimization::revoke_handshake(JavaThread* thread, frame fr,
> RegisterMap* map) {
> Perhaps revoke_using_handshake().
Changed.
>
> old L1325: ObjectSynchronizer::inflate(thread, obj,
> ObjectSynchronizer::inflate_cause_vm_internal);
> Why was this deleted?
> Update: See synchronizer.cpp below.
>
> src/hotspot/share/runtime/deoptimization.hpp
> No comments.
>
> src/hotspot/share/runtime/synchronizer.cpp
> Deleting ObjectSynchronizer::inflate() in
> Deoptimization::inflate_monitors_handshake()
> which is now Deoptimization::revoke_handshake() allows these changes
> to be reverted...
Yes, the JavaThread will inflate, if needed, when unpackning the stack preparing
for interpreter.
>
> test/hotspot/jtreg/compiler/codecache/stress/UnexpectedDeoptimizationAllTest.java
> L2: * Copyright (c) 2014, 2016, Oracle and/or its affiliates. All rights
> reserved.
> This is listed as a new file, but it has an old (and dual) copyright year.
Fixed.
>
> Do any of the options used in the test require non-product bits?
> (I don't think so, but...)
I don't know if WB works with release build.
Thanks!
/Robbin
>
> Dan
>
>
> On 5/15/19 2:26 AM, Robbin Ehn wrote:
>> Hi, please see this update.
>>
>> I think I got all review comments fix.
>>
>> Long story short, I was concerned about test coverage, so I added a stress test
>> using the WB, which sometimes crashed in rubbish code.
>>
>> There are two bugs in the methods used by WB_DeoptimizeAll.
>> (Seems I'm the first user)
>>
>> CodeCache::mark_all_nmethods_for_deoptimization();
>> When iterating the nmethods we could see the methods being create in:
>> void AdapterHandlerLibrary::create_native_wrapper(const methodHandle& method)
>> And deopt the method when it was in use or before.
>> Native wrappers are suppose to live as long as the class.
>> I filtered out not_installed and native methods.
>>
>> Deoptimization::deoptimize_all_marked();
>> The issue is that a not_entrant method can go to zombie at anytime.
>> There are several ways to make a nmethod not go zombie: nmethodLocker, have it
>> on stack, avoid safepoint poll in some states, etc.., which is also depending on
>> what type of nmethod.
>> The iterator only_alive_and_not_unloading returns not_entrant nmethods, but we
>> don't know there state prior last poll.
>> in_use -> not_entrant -> #poll# -> not_entrant -> zombie
>> If the iterator returns the nmethod after we passed the poll it can still be
>> not_entrant but go zombie.
>> The problem happens when a second thread marks a method for deopt and makes it
>> not_entrant. Then after a poll we end-up in deoptimize_all_marked(), but the
>> method is not yet a zombie, so the iterator returns it, it becomes a zombie thus
>> pass the if check and later hit the assert.
>> So there is a race between the iterator check of state and if-statement check of
>> state. Fixed by also filtering out zombies.
>>
>> If the stress test with correction of the bugs causes trouble in review, I can
>> do a follow-up with the stress test separately.
>>
>> Good news, no issues found with deopt with handshakes.
>>
>> This is v3:
>> http://cr.openjdk.java.net/~rehn/8221734/v3/webrev/
>>
>> This full inc from v2 (review + stress test):
>> http://cr.openjdk.java.net/~rehn/8221734/v3/inc/
>>
>> This inc is the review part from v2:
>> http://cr.openjdk.java.net/~rehn/8221734/v3/inc_review/
>>
>> This inc is the additional stress test with bug fixes:
>> http://cr.openjdk.java.net/~rehn/8221734/v3/inc_test/
>>
>> Additional biased locking change:
>> The original code use same copy of markOop in revoke_and_rebias.
>> The keep same behavior I now pass in that copy into fast_revoke.
>>
>> The stress test passes hundreds of iterations in mach5.
>> Thousands stress tests locally, the issues above was reproduce-able.
>> Inc changes also passes t1-5.
>>
>> As usual with this change-set, I'm continuously running more test.
>>
>> Thanks, Robbin
>>
>> On 2019-04-25 14:05, 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
>
More information about the hotspot-compiler-dev
mailing list