RFR(m): 8221734: Deoptimize with handshakes
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon May 20 22:33:09 UTC 2019
> This full inc from v2 (review + stress test):
> http://cr.openjdk.java.net/~rehn/8221734/v3/inc/
src/hotspot/share/aot/aotCompiledMethod.cpp
No comments.
src/hotspot/share/aot/aotCompiledMethod.hpp
No comments.
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).
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.
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.
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.
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().
L1311: void Deoptimization::revoke_handshake(JavaThread* thread,
frame fr, RegisterMap* map) {
Perhaps revoke_using_handshake().
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...
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.
Do any of the options used in the test require non-product bits?
(I don't think so, but...)
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