Hi,
this is webrev.5 based on Robbin's feedback and Martin's review - thanks! :)
The change affects jvmti, hotspot and c2. Partial reviews are very welcome too.
Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5/
Delta: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5.inc/
Robbin, Martin, please let me know, if anything shouldn't be quite as you
wanted it. Also find my
comments on your feedback below.
Robbin, can I count you as Reviewer for the runtime part?
Thanks, Richard.
--
DeoptimizeObjectsALotThread is only used in compileBroker.cpp.
You can move both declaration and definition to that file, no need to clobber
thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry)
Done.
Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's own
hpp file? It doesn't seem right to add JVM TI classes into thread.hpp.
I moved JvmtiDeferredUpdates to vframe_hp.hpp where preexisting
jvmtiDeferredLocalVariableSet is
declared.
src/hotspot/share/code/compiledMethod.cpp
Nice cleanup!
Thanks :)
src/hotspot/share/code/debugInfoRec.cpp
src/hotspot/share/code/debugInfoRec.hpp
Additional parmeters. (Remark: I think "non_global_escape_in_scope" would read better
than "not_global_escape_in_scope", but your version is consistent with existing code, so
no change request from my side.) Ok.
I've been thinking about this too and finally stayed with
not_global_escape_in_scope. It's supposed
to mean an object whose escape state is not GlobalEscape is in scope.
src/hotspot/share/compiler/compileBroker.cpp
src/hotspot/share/compiler/compileBroker.hpp
Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a
follow up change together with the test in order to make this webrev smaller,
but since it is included, I'm reviewing everything at once. Not a big deal.) Ok.
Yes the change would be a little smaller. And if it helps I'll split it off. In
general I prefer
patches that bring along a suitable amount of tests.
src/hotspot/share/opto/c2compiler.cpp
Make do_escape_analysis independent of JVMCI capabilities. Nice!
It is the main goal of the enhancement. It is done for C2, but could be done
for JVMCI compilers
with just a small effort as well.
src/hotspot/share/opto/escape.cpp
Annotation for MachSafePointNodes. Your added functionality looks correct.
But I'd prefer to move the bulky code out of the large function.
I suggest to factor out something like has_not_global_escape and
has_arg_escape. So the code could look like this:
SafePointNode* sfn = sfn_worklist.at(next);
sfn->set_not_global_escape_in_scope(has_not_global_escape(sfn));
if (sfn->is_CallJava()) {
CallJavaNode* call = sfn->as_CallJava();
call->set_arg_escape(has_arg_escape(call));
}
This would also allow us to get rid of the found_..._escape_in_args variables
making the loops better readable.
Done.
It's kind of ugly to use strcmp to recognize uncommon trap, but that seems to
be the way to do it (there are more such places). So it's ok.
Yeah. I copied the snippet.
src/hotspot/share/prims/jvmtiImpl.cpp
src/hotspot/share/prims/jvmtiImpl.hpp
The sequence is pretty complex:
VM_GetOrSetLocal element initialization executes EscapeBarrier code which
suspends the target thread (extra VM Operation).
Note that the target threads have to be suspended already for
VM_GetOrSetLocal*. So it's mainly the
synchronization effect of EscapeBarrier::sync_and_suspend_one() that is
required here. Also no extra
_handshake_ is executed, since sync_and_suspend_one() will find the target
threads already
suspended.
VM_GetOrSetLocal::doit_prologue performs object deoptimization (by VM Thread to
prepare VM Operation with frame deoptimization).
VM_GetOrSetLocal destructor implicitly calls EscapeBarrier destructor which
resumes the target thread.
But I don't have any improvement proposal. Performance is probably not a
concern, here. So it's ok.
VM_GetOrSetLocal::deoptimize_objects deoptimizes the top frame if it has
non-globally escaping objects and other frames if they have arg escaping ones.
Good.
It's not specifically the top frame, but the frame that is accessed.
src/hotspot/share/runtime/deoptimization.cpp
Object deoptimization. I have more comments and proposals, here.
First of all, handling recursive and waiting locks in relock_objects is tricky,
but looks correct.
Comments are sufficient to understand why things are done as they are
implemented.
BiasedLocking related parts are complex, but we may get rid of them in the
future (with BiasedLocking removal).
Anyway, looks correct, too.
Typo in comment: "regularily" => "regularly"
Deoptimization::fetch_unroll_info_helper is the only place where
_jvmti_deferred_updates get deallocated (except JavaThread destructor). But I
think we always go through it, so I can't see a memory leak or such kind of
issues.
That's correct. The compiled frame for which deferred updates are allocated is
always deoptimized
before (see EscapeBarrier::deoptimize_objects()). This is also asserted in
compiledVFrame::update_deferred_value(). I've added the same assertion to
Deoptimization::relock_objects(). So we can be sure that
_jvmti_deferred_updates are deallocated
again in fetch_unroll_info_helper().
EscapeBarrier::deoptimize_objects: ResourceMark should use calling_thread().
Sure, well spotted!
You can use MutexLocker and MonitorLocker with Thread* to save the
Thread::current() call.
Right, good hint. This was recently introduced with 8235678. I even had to
resolve conflicts. Should
have done this then.
I'd make set_objs_are_deoptimized static and remove it from the EscapeBarrier
interface because I think it shouldn't be used outside of
EscapeBarrier::deoptimize_objects.
Done.
Typo in comment: "we must only deoptimize" => "we only have to deoptimize"
Replaced with "[...] we deoptimize iff local objects are passed as args"
"bool EscapeBarrier::deoptimize_objects(intptr_t* fr_id)" is trivial and
barrier_active() is redundant. Implementation can get moved to hpp file.
Ok. Done.
I'll get back to suspend flags, later.
There are weird cases regarding _self_deoptimization_in_progress.
Assume we have 3 threads A, B and C. A deopts C, B deopts C, C deopts C. C can
set _self_deoptimization_in_progress while A performs the handshake for
suspending C. I think this doesn't lead to errors, but it's probably not
desired.
I think it would be better to use only one "wait" call in sync_and_suspend_one
and sync_and_suspend_all.
You're right. We've discussed that face-to-face, but couldn't find a real
issue. But now, thinking again, a reckon I found one:
2808 // Sync with other threads that might be doing deoptimizations
2809 {
2810 // Need to switch to _thread_blocked for the wait() call
2811 ThreadBlockInVM tbivm(_calling_thread);
2812 MonitorLocker ml(EscapeBarrier_lock, Mutex::_no_safepoint_check_flag);
2813 while (_self_deoptimization_in_progress) {
2814 ml.wait();
2815 }
2816
2817 if (self_deopt()) {
2818 _self_deoptimization_in_progress = true;
2819 }
2820
2821 while (_deoptee_thread->is_ea_obj_deopt_suspend()) {
2822 ml.wait();
2823 }
2824
2825 if (self_deopt()) {
2826 return;
2827 }
2828
2829 // set suspend flag for target thread
2830 _deoptee_thread->set_ea_obj_deopt_flag();
2831 }
- A waits in 2822
- C is suspended
- B notifies all in resume_one()
- A and C wake up
- C wins over A and sets _self_deoptimization_in_progress = true in 2818
- C does the self deoptimization
- A executes 2830 _deoptee_thread->set_ea_obj_deopt_flag()
C will self suspend at some undefined point. The resulting state is illegal.
I first thought it'd be better to move ThreadBlockInVM before wait() to reduce
thread state transitions, but that seems to be problematic because
ThreadBlockInVM destructor contains a safepoint check which we shouldn't do
while holding EscapeBarrier_lock. So no change request.
Yes, would be nice to have the state change only if needed, but for the reason
you mentioned it is
not quite as easy as it seems to be. I experimented as well with a second lock,
but did not succeed.
Change in thred_added:
I think the sequence would be more comprehensive if we waited for
deopt_all_threads in Thread::start and all other places where a new thread can
run into Java code (e.g. JVMTI attach).
Your version makes new threads come up with suspend flag set. That looks
correct, too. Advantage is that you only have to change one place
(thread_added). It'll be interesting to see how it will look like when we use
async handshakes instead of suspend flags.
For now, I'm ok with your version.
I had a version that did what you are suggesting. The current version also has
the advantage, that
there are fewer places where a thread has to wait for ongoing object
deoptimization. This means
viewer places where you have to worry about correct thread state transitions,
possible deadlocks,
and if all oops are properly Handle'ed.
I'd only move MutexLocker ml(EscapeBarrier_lock...) after if
(!jt->is_hidden_from_external_view()).
Done.
Having 4 different deoptimize_objects functions makes it a little hard to keep
an overview of which one is used for what.
Maybe adding suffixes would help a little bit, but I can also live with what
you have.
Implementation looks correct to me.
2 are internal. I added the suffix _internal to them. This leaves 2 to choose
from.
src/hotspot/share/runtime/deoptimization.hpp
Escape barriers and object deoptimization functions.
Typo in comment: "helt" => "held"
Done in place already.
src/hotspot/share/runtime/interfaceSupport.cpp
InterfaceSupport::deoptimizeAllObjects() is only used for DeoptimizeObjectsALot
= 1.
I think DeoptimizeObjectsALot = 2 is more important, but I think it's not bad
to have DeoptimizeObjectsALot = 1 in addition. Ok.
I never used DeoptimizeObjectsALot = 1 that much. It could be more
deterministic in single threaded
scenarios. I wouldn't object to get rid of it though.
src/hotspot/share/runtime/stackValue.hpp
Better reinitilization in StackValue. Good.
StackValue::obj_is_scalar_replaced() should not return true after calling
set_obj().
src/hotspot/share/runtime/thread.cpp
src/hotspot/share/runtime/thread.hpp
src/hotspot/share/runtime/thread.inline.hpp
wait_for_object_deoptimization, suspend flag, deferred updates and test feature
to deoptimize objects.
In the long term, we want to get rid of suspend flags, so it's not so nice to
introduce a new one. But I agree with Götz that it should be acceptable as
temporary solution until async handshakes are available (which takes more
time). So I'm ok with your change.
I'm keen to build the feature on async handshakes when the arive.
You can use MutexLocker with Thread*.
Done.
JVMTIDeferredUpdates: I agree with Robin. It'd be nice to move the class out of
thread.hpp.
Done.
src/hotspot/share/runtime/vframe.cpp
Added support for entry frame to new_vframe. Ok.
src/hotspot/share/runtime/vframe_hp.cpp
src/hotspot/share/runtime/vframe_hp.hpp
I think code()->as_nmethod() in not_global_escape_in_scope() and arg_escape()
should better be under #ifdef ASSERT or inside the assert statement (no need for
code cache walking in product build).
Done.
jvmtiDeferredLocalVariableSet::update_monitors:
Please add a comment explaining that owner referenced by original info may be
scalar replaced, but it is deoptimized in the vframe.
Done.
-----Original Message-----
From: Doerr, Martin <martin.do...@sap.com>
Sent: Donnerstag, 12. März 2020 17:28
To: Reingruber, Richard <richard.reingru...@sap.com>; 'Robbin Ehn' <robbin....@oracle.com>;
Lindenmaier, Goetz <goetz.lindenma...@sap.com>; David Holmes <david.hol...@oracle.com>; Vladimir
Kozlov (vladimir.koz...@oracle.com) <vladimir.koz...@oracle.com>; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in
the Presence of JVMTI Agents
Hi Richard,
I managed to find time for a (almost) complete review of webrev.4. (I'll review
the tests separately.)
First of all, the change seems to be in pretty good quality for its significant
complexity. I couldn't find any real bugs. But I'd like to propose minor
improvements.
I'm convinced that it's mature because we did substantial testing.
I like the new functionality for object deoptimization. It can possibly be
reused for future escape analysis based optimizations. So I appreciate having
it available in the code base.
In addition to that, your change makes the JVMTI implementation better
integrated into the VM.
Now to the details:
src/hotspot/share/c1/c1_IR.hpp
describe_scope parameters. Ok.
src/hotspot/share/ci/ciEnv.cpp
src/hotspot/share/ci/ciEnv.hpp
Fix for JvmtiExport::can_walk_any_space() capability. Ok.
src/hotspot/share/code/compiledMethod.cpp
Nice cleanup!
src/hotspot/share/code/debugInfoRec.cpp
src/hotspot/share/code/debugInfoRec.hpp
Additional parmeters. (Remark: I think "non_global_escape_in_scope" would read better
than "not_global_escape_in_scope", but your version is consistent with existing code, so
no change request from my side.) Ok.
src/hotspot/share/code/nmethod.cpp
Nice cleanup!
src/hotspot/share/code/pcDesc.hpp
Additional parameters. Ok.
src/hotspot/share/code/scopeDesc.cpp
src/hotspot/share/code/scopeDesc.hpp
Improved implementation + additional parameters. Ok.
src/hotspot/share/compiler/compileBroker.cpp
src/hotspot/share/compiler/compileBroker.hpp
Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a
follow up change together with the test in order to make this webrev smaller,
but since it is included, I'm reviewing everything at once. Not a big deal.) Ok.
src/hotspot/share/jvmci/jvmciCodeInstaller.cpp
Additional parameters. Ok.
src/hotspot/share/opto/c2compiler.cpp
Make do_escape_analysis independent of JVMCI capabilities. Nice!
src/hotspot/share/opto/callnode.hpp
Additional fields for MachSafePointNodes. Ok.
src/hotspot/share/opto/escape.cpp
Annotation for MachSafePointNodes. Your added functionality looks correct.
But I'd prefer to move the bulky code out of the large function.
I suggest to factor out something like has_not_global_escape and
has_arg_escape. So the code could look like this:
SafePointNode* sfn = sfn_worklist.at(next);
sfn->set_not_global_escape_in_scope(has_not_global_escape(sfn));
if (sfn->is_CallJava()) {
CallJavaNode* call = sfn->as_CallJava();
call->set_arg_escape(has_arg_escape(call));
}
This would also allow us to get rid of the found_..._escape_in_args variables
making the loops better readable.
It's kind of ugly to use strcmp to recognize uncommon trap, but that seems to
be the way to do it (there are more such places). So it's ok.
src/hotspot/share/opto/machnode.hpp
Additional fields for MachSafePointNodes. Ok.
src/hotspot/share/opto/macro.cpp
Allow elimination of non-escaping allocations. Ok.
src/hotspot/share/opto/matcher.cpp
src/hotspot/share/opto/output.cpp
Copy attribute / pass parameters. Ok.
src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp
Nice cleanup!
src/hotspot/share/prims/jvmtiEnv.cpp
src/hotspot/share/prims/jvmtiEnvBase.cpp
Escape barriers + deoptimize objects for target thread. Good.
src/hotspot/share/prims/jvmtiImpl.cpp
src/hotspot/share/prims/jvmtiImpl.hpp
The sequence is pretty complex:
VM_GetOrSetLocal element initialization executes EscapeBarrier code which
suspends the target thread (extra VM Operation).
VM_GetOrSetLocal::doit_prologue performs object deoptimization (by VM Thread to
prepare VM Operation with frame deoptimization).
VM_GetOrSetLocal destructor implicitly calls EscapeBarrier destructor which
resumes the target thread.
But I don't have any improvement proposal. Performance is probably not a
concern, here. So it's ok.
VM_GetOrSetLocal::deoptimize_objects deoptimizes the top frame if it has
non-globally escaping objects and other frames if they have arg escaping ones.
Good.
src/hotspot/share/prims/jvmtiTagMap.cpp
Escape barriers + deoptimize objects for all threads. Ok.
src/hotspot/share/prims/whitebox.cpp
Added WB_IsFrameDeoptimized to API. Ok.
src/hotspot/share/runtime/deoptimization.cpp
Object deoptimization. I have more comments and proposals, here.
First of all, handling recursive and waiting locks in relock_objects is tricky,
but looks correct.
Comments are sufficient to understand why things are done as they are
implemented.
BiasedLocking related parts are complex, but we may get rid of them in the
future (with BiasedLocking removal).
Anyway, looks correct, too.
Typo in comment: "regularily" => "regularly"
Deoptimization::fetch_unroll_info_helper is the only place where
_jvmti_deferred_updates get deallocated (except JavaThread destructor). But I
think we always go through it, so I can't see a memory leak or such kind of
issues.
EscapeBarrier::deoptimize_objects: ResourceMark should use calling_thread().
You can use MutexLocker and MonitorLocker with Thread* to save the
Thread::current() call.
I'd make set_objs_are_deoptimized static and remove it from the EscapeBarrier
interface because I think it shouldn't be used outside of
EscapeBarrier::deoptimize_objects.
Typo in comment: "we must only deoptimize" => "we only have to deoptimize"
"bool EscapeBarrier::deoptimize_objects(intptr_t* fr_id)" is trivial and
barrier_active() is redundant. Implementation can get moved to hpp file.
I'll get back to suspend flags, later.
There are weird cases regarding _self_deoptimization_in_progress.
Assume we have 3 threads A, B and C. A deopts C, B deopts C, C deopts C. C can
set _self_deoptimization_in_progress while A performs the handshake for
suspending C. I think this doesn't lead to errors, but it's probably not
desired.
I think it would be better to use only one "wait" call in sync_and_suspend_one
and sync_and_suspend_all.
I first thought it'd be better to move ThreadBlockInVM before wait() to reduce
thread state transitions, but that seems to be problematic because
ThreadBlockInVM destructor contains a safepoint check which we shouldn't do
while holding EscapeBarrier_lock. So no change request.
Change in thred_added:
I think the sequence would be more comprehensive if we waited for
deopt_all_threads in Thread::start and all other places where a new thread can
run into Java code (e.g. JVMTI attach).
Your version makes new threads come up with suspend flag set. That looks
correct, too. Advantage is that you only have to change one place
(thread_added). It'll be interesting to see how it will look like when we use
async handshakes instead of suspend flags.
For now, I'm ok with your version.
I'd only move MutexLocker ml(EscapeBarrier_lock...) after if
(!jt->is_hidden_from_external_view()).
Having 4 different deoptimize_objects functions makes it a little hard to keep
an overview of which one is used for what.
Maybe adding suffixes would help a little bit, but I can also live with what
you have.
Implementation looks correct to me.
src/hotspot/share/runtime/deoptimization.hpp
Escape barriers and object deoptimization functions.
Typo in comment: "helt" => "held"
src/hotspot/share/runtime/globals.hpp
Addition of develop flag DeoptimizeObjectsALotInterval. Ok.
src/hotspot/share/runtime/interfaceSupport.cpp
InterfaceSupport::deoptimizeAllObjects() is only used for DeoptimizeObjectsALot
= 1.
I think DeoptimizeObjectsALot = 2 is more important, but I think it's not bad
to have DeoptimizeObjectsALot = 1 in addition. Ok.
src/hotspot/share/runtime/interfaceSupport.inline.hpp
Addition of deoptimizeAllObjects. Ok.
src/hotspot/share/runtime/mutexLocker.cpp
src/hotspot/share/runtime/mutexLocker.hpp
Addition of EscapeBarrier_lock. Ok.
src/hotspot/share/runtime/objectMonitor.cpp
Make recursion count relock aware. Ok.
src/hotspot/share/runtime/stackValue.hpp
Better reinitilization in StackValue. Good.
src/hotspot/share/runtime/thread.cpp
src/hotspot/share/runtime/thread.hpp
src/hotspot/share/runtime/thread.inline.hpp
wait_for_object_deoptimization, suspend flag, deferred updates and test feature
to deoptimize objects.
In the long term, we want to get rid of suspend flags, so it's not so nice to
introduce a new one. But I agree with Götz that it should be acceptable as
temporary solution until async handshakes are available (which takes more
time). So I'm ok with your change.
You can use MutexLocker with Thread*.
JVMTIDeferredUpdates: I agree with Robin. It'd be nice to move the class out of
thread.hpp.
src/hotspot/share/runtime/vframe.cpp
Added support for entry frame to new_vframe. Ok.
src/hotspot/share/runtime/vframe_hp.cpp
src/hotspot/share/runtime/vframe_hp.hpp
I think code()->as_nmethod() in not_global_escape_in_scope() and arg_escape()
should better be under #ifdef ASSERT or inside the assert statement (no need for
code cache walking in product build).
jvmtiDeferredLocalVariableSet::update_monitors:
Please add a comment explaining that owner referenced by original info may be
scalar replaced, but it is deoptimized in the vframe.
src/hotspot/share/utilities/macros.hpp
Addition of NOT_COMPILER2_OR_JVMCI_RETURN macros. Ok.
test/hotspot/jtreg/serviceability/jvmti/Heap/IterateHeapWithEscapeAnalysisEnabled.java
test/hotspot/jtreg/serviceability/jvmti/Heap/libIterateHeapWithEscapeAnalysisEnabled.c
New test. Will review separately.
test/jdk/TEST.ROOT
Addition of vm.jvmci as required property. Ok.
test/jdk/com/sun/jdi/EATests.java
test/jdk/com/sun/jdi/EATestsJVMCI.java
New test. Will review separately.
test/lib/sun/hotspot/WhiteBox.java
Added isFrameDeoptimized to API. Ok.
That was it. Best regards,
Martin
-----Original Message-----
From: hotspot-compiler-dev <hotspot-compiler-dev-
boun...@openjdk.java.net> On Behalf Of Reingruber, Richard
Sent: Dienstag, 3. März 2020 21:23
To: 'Robbin Ehn' <robbin....@oracle.com>; Lindenmaier, Goetz
<goetz.lindenma...@sap.com>; David Holmes <david.hol...@oracle.com>;
Vladimir Kozlov (vladimir.koz...@oracle.com)
<vladimir.koz...@oracle.com>; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-runtime-
d...@openjdk.java.net
Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better
Performance in the Presence of JVMTI Agents
Hi Robbin,
I understand that Robbin proposed to replace the usage of
_suspend_flag with handshakes. Apparently, async handshakes
are needed to do so. We have been waiting a while for removal
of the _suspend_flag / introduction of async handshakes [2].
What is the status here?
I have an old prototype which I would like to continue to work on.
So do not assume asynch handshakes will make 15.
Even if it would, I think there are a lot more investigate work to remove
_suspend_flag.
Let us know, if we can be of any help to you and be it only testing.
Full:
http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/
DeoptimizeObjectsALotThread is only used in compileBroker.cpp.
You can move both declaration and definition to that file, no need to
clobber
thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry)
Will do.
Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's
own
hpp file? It doesn't seem right to add JVM TI classes into thread.hpp.
You are right. It shouldn't be declared in thread.hpp. I will look into that.
Note that we also think we may have a bug in deopt:
https://bugs.openjdk.java.net/browse/JDK-8238237
I think it would be best, if possible, to push after that is resolved.
Sure.
Not even nearly a full review :)
I know :)
Anyways, thanks a lot,
Richard.
-----Original Message-----
From: Robbin Ehn <robbin....@oracle.com>
Sent: Monday, March 2, 2020 11:17 AM
To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Reingruber, Richard
<richard.reingru...@sap.com>; David Holmes <david.hol...@oracle.com>;
Vladimir Kozlov (vladimir.koz...@oracle.com)
<vladimir.koz...@oracle.com>; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-runtime-
d...@openjdk.java.net
Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance
in the Presence of JVMTI Agents
Hi,
On 2/24/20 5:39 PM, Lindenmaier, Goetz wrote:
Hi,
I had a look at the progress of this change. Nothing
happened since Richard posted his update using more
handshakes [1].
But we (SAP) would appreciate a lot if this change could
be successfully reviewed and pushed.
I think there is basic understanding that this
change is helpful. It fixes a number of issues with JVMTI,
and will deliver the same performance benefits as EA
does in current production mode for debugging scenarios.
This is important for us as we run our VMs prepared
for debugging in production mode.
I understand that Robbin proposed to replace the usage of
_suspend_flag with handshakes. Apparently, async handshakes
are needed to do so. We have been waiting a while for removal
of the _suspend_flag / introduction of async handshakes [2].
What is the status here?
I have an old prototype which I would like to continue to work on.
So do not assume asynch handshakes will make 15.
Even if it would, I think there are a lot more investigate work to remove
_suspend_flag.
I think we should no longer wait, but proceed with
this change. We will look into removing the usage of
suspend_flag introduced here once it is possible to implement
it with handshakes.
Yes, sure.
Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4/
DeoptimizeObjectsALotThread is only used in compileBroker.cpp.
You can move both declaration and definition to that file, no need to clobber
thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry)
Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's
own
hpp file? It doesn't seem right to add JVM TI classes into thread.hpp.
Note that we also think we may have a bug in deopt:
https://bugs.openjdk.java.net/browse/JDK-8238237
I think it would be best, if possible, to push after that is resolved.
Not even nearly a full review :)
Thanks, Robbin
Incremental:
http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.4.inc/
I was not able to eliminate the additional suspend flag now. I'll take care
of this
as soon as the
existing suspend-resume-mechanism is reworked.
Testing:
Nightly tests @SAP:
JCK and JTREG, also in Xcomp mode, SPECjvm2008, SPECjbb2015,
Renaissance
Suite, SAP specific tests
with fastdebug and release builds on all platforms
Stress testing with DeoptimizeObjectsALot running SPECjvm2008 40x
parallel
for 24h
Thanks, Richard.
More details on the changes:
* Hide DeoptimizeObjectsALotThread from external view.
* Changed EscapeBarrier_lock to be a _safepoint_check_never lock.
It used to be _safepoint_check_sometimes, which will be eliminated
sooner or
later.
I added explicit thread state changes with ThreadBlockInVM to code
paths
where we can wait()
on EscapeBarrier_lock to become safepoint safe.
* Use handshake EscapeBarrierSuspendHandshake to suspend target
threads
instead of vm operation
VM_ThreadSuspendAllForObjDeopt.
* Removed uses of Threads_lock. When adding a new thread we suspend
it iff
EA optimizations are
being reverted. In the previous version we were waiting on
Threads_lock
while EA optimizations
were reverted. See EscapeBarrier::thread_added().
* Made tests require Xmixed compilation mode.
* Made tests agnostic regarding tiered compilation.
I.e. tc isn't disabled anymore, and the tests can be run with tc enabled or
disabled.
* Exercising EATests.java as well with stress test options
DeoptimizeObjectsALot*
Due to the non-deterministic deoptimizations some tests need to be
skipped.
We do this to prevent bit-rot of the stress test code.
* Executing EATests.java as well with graal if available. Driver for this is
EATestsJVMCI.java. Graal cannot pass all tests, because it does not
provide all
the new debug info
(namely not_global_escape_in_scope and arg_escape in
scopeDesc.hpp).
And graal does not yet support the JVMTI operations force early return
and
pop frame.
* Removed tracing from new jdi tests in EATests.java. Too much trace
output
before the debugging
connection is established can cause deadlock because output buffers fill
up.
(See https://bugs.openjdk.java.net/browse/JDK-8173304)
* Many copyright year changes and smaller clean-up changes of testing
code
(trailing white-space and
the like).
-----Original Message-----
From: David Holmes <david.hol...@oracle.com>
Sent: Donnerstag, 19. Dezember 2019 03:12
To: Reingruber, Richard <richard.reingru...@sap.com>; serviceability-
d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-
runtime-...@openjdk.java.net; Vladimir Kozlov
(vladimir.koz...@oracle.com)
<vladimir.koz...@oracle.com>
Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better
Performance in
the Presence of JVMTI Agents
Hi Richard,
I think my issue is with the way EliminateNestedLocks works so I'm going
to look into that more deeply.
Thanks for the explanations.
David
On 18/12/2019 12:47 am, Reingruber, Richard wrote:
Hi David,
> > > Some further queries/concerns:
> > >
> > > src/hotspot/share/runtime/objectMonitor.cpp
> > >
> > > Can you please explain the changes to ObjectMonitor::wait:
> > >
> > > ! _recursions = save // restore the old recursion count
> > > ! + jt->get_and_reset_relock_count_after_wait();
//
> > > increased by the deferred relock count
> > >
> > > what is the "deferred relock count"? I gather it relates to
> > >
> > > "The code was extended to be able to deoptimize objects of a
> > frame that
> > > is not the top frame and to let another thread than the owning
> > thread do
> > > it."
> >
> > Yes, these relate. Currently EA based optimizations are reverted,
when a
compiled frame is
> > replaced with corresponding interpreter frames. Part of this is
relocking
objects with eliminated
> > locking. New with the enhancement is that we do this also just
before
object references are
> > acquired through JVMTI. In this case we deoptimize also the
owning
compiled frame C and we
> > register deoptimized objects as deferred updates. When control
returns
to C it gets deoptimized,
> > we notice that objects are already deoptimized (reallocated and
relocked), so we don't do it again
> > (relocking twice would be incorrect of course). Deferred updates
are
copied into the new
> > interpreter frames.
> >
> > Problem: relocking is not possible if the target thread T is waiting
on the
monitor that needs to
> > be relocked. This happens only with non-local objects with
EliminateNestedLocks. Instead relocking
> > is deferred until T owns the monitor again. This is what the piece of
code above does.
>
> Sorry I need some more detail here. How can you wait() on an
object
> monitor if the object allocation and/or locking was optimised away?
And
> what is a "non-local object" in this context? Isn't EA restricted to
> thread-confined objects?
"Non-local object" is an object that escapes its thread. The issue I'm
addressing with the changes
in ObjectMonitor::wait are almost unrelated to EA. They are caused by
EliminateNestedLocks, where C2
eliminates recursive locking of an already owned lock. The lock owning
object
exists on the heap, it
is locked and you can call wait() on it.
EliminateLocks is the C2 option that controls lock elimination based on
EA.
Both optimizations have
in common that objects with eliminated locking need to be relocked
when
deoptimizing a frame,
i.e. when replacing a compiled frame with equivalent interpreter
frames. Deoptimization::relock_objects does that job for /all/ eliminated
locks in scope. /All/ can
be a mix of eliminated nested locks and locks of not-escaping objects.
New with the enhancement: I call relock_objects earlier, just before
objects
pontentially
escape. But then later when the owning compiled frame gets
deoptimized, I
must not do it again:
See call to EscapeBarrier::objs_are_deoptimized in deoptimization.cpp:
373 if ((jvmci_enabled || ((DoEscapeAnalysis ||
EliminateNestedLocks) &&
EliminateLocks))
374 && !EscapeBarrier::objs_are_deoptimized(thread,
deoptee.id())) {
375 bool unused;
376 eliminate_locks(thread, chunk, realloc_failures, deoptee,
exec_mode,
unused);
377 }
Now when calling relock_objects early it is quiet possible that I have to
relock
an object the
target thread currently waits for. Obviously I cannot relock in this case,
instead I chose to
introduce relock_count_after_wait to JavaThread.
> Is it just that some of the locking gets optimized away e.g.
>
> synchronised(obj) {
> synchronised(obj) {
> synchronised(obj) {
> obj.wait();
> }
> }
> }
>
> If this is reduced to a form as-if it were a single lock of the monitor
> (due to EA) and the wait() triggers a JVM TI event which leads to the
> escape of "obj" then we need to reconstruct the true lock state, and
so
> when the wait() internally unblocks and reacquires the monitor it
has to
> set the true recursion count to 3, not the 1 that it appeared to be
when
> wait() was initially called. Is that the scenario?
Kind of... except that the locking is not eliminated due to EA and there is
no
JVM TI event
triggered by wait.
Add
LocalObject l1 = new LocalObject();
in front of the synchrnized blocks and assume a JVM TI agent acquires l1.
This
triggers the code in
question.
See that relocking/reallocating is transactional. If it is done then for /all/
objects in scope and it is
done at most once. It wouldn't be quite so easy to split this in relocking
of
nested/EA-based
eliminated locks.
> If so I find this truly awful. Anyone using wait() in a realistic form
> requires a notification and so the object cannot be thread confined.
In
It is not thread confined.
> which case I would strongly argue that upon hitting the wait() the
deopt
> should occur unconditionally and so the lock state is correct before
we
> wait and so we don't need to mess with the recursion count
internally
> when we reacquire the monitor.
>
> >
> > > which I don't like the sound of at all when it comes to
ObjectMonitor
> > > state. So I'd like to understand in detail exactly what is going
on
here
> > > and why. This is a very intrusive change that seems to badly
break
> > > encapsulation and impacts future changes to ObjectMonitor
that are
under
> > > investigation.
> >
> > I would not regard this as breaking encapsulation. Certainly not
badly.
> >
> > I've added a property relock_count_after_wait to JavaThread. The
property is well
> > encapsulated. Future ObjectMonitor implementations have to deal
with
recursion too. They are free
> > in choosing a way to do that as long as that property is taken into
account. This is hardly a
> > limitation.
>
> I do think this badly breaks encapsulation as you have to add a
callout
> from the guts of the ObjectMonitor code to reach into the thread to
get
> this lock count adjustment. I understand why you have had to do
this but
> I would much rather see a change to the EA optimisation strategy so
that
> this is not needed.
>
> > Note also that the property is a straight forward extension of the
existing concept of deferred
> > local updates. It is embedded into the structure holding them. So
not
even the footprint of a
> > JavaThread is enlarged if no deferred updates are generated.
>
> [...]
>
> >
> > I'm actually duplicating the existing external suspend mechanism,
because a thread can be
> > suspended at most once. And hey, and don't like that either! But it
seems not unlikely that the
> > duplicate can be removed together with the original and the new
type
of handshakes that will be
> > used for thread suspend can be used for object deoptimization
too. See
today's discussion in
> > JDK-8227745 [2].
>
> I hope that discussion bears some fruit, at the moment it seems not
to
> be possible to use handshakes here. :(
>
> The external suspend mechanism is a royal pain in the proverbial
that we
> have to carefully live with. The idea that we're duplicating that for
> use in another fringe area of functionality does not thrill me at all.
>
> To be clear, I understand the problem that exists and that you wish
to
> solve, but for the runtime parts I balk at the complexity cost of
> solving it.
I know it's complex, but by far no rocket science.
Also I find it hard to imagine another fix for JDK-8233915 besides
changing
the JVM TI specification.
Thanks, Richard.
-----Original Message-----
From: David Holmes <david.hol...@oracle.com>
Sent: Dienstag, 17. Dezember 2019 08:03
To: Reingruber, Richard <richard.reingru...@sap.com>; serviceability-
d...@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-
runtime-...@openjdk.java.net; Vladimir Kozlov
(vladimir.koz...@oracle.com)
<vladimir.koz...@oracle.com>
Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better
Performance
in the Presence of JVMTI Agents
<resend as my mailer crashed during last send>
David
On 17/12/2019 4:57 pm, David Holmes wrote:
Hi Richard,
On 14/12/2019 5:01 am, Reingruber, Richard wrote:
Hi David,
> Some further queries/concerns:
>
> src/hotspot/share/runtime/objectMonitor.cpp
>
> Can you please explain the changes to ObjectMonitor::wait:
>
> ! _recursions = save // restore the old recursion count
> ! + jt->get_and_reset_relock_count_after_wait(); //
> increased by the deferred relock count
>
> what is the "deferred relock count"? I gather it relates to
>
> "The code was extended to be able to deoptimize objects of a
frame that
> is not the top frame and to let another thread than the owning
thread do
> it."
Yes, these relate. Currently EA based optimizations are reverted,
when
a compiled frame is replaced
with corresponding interpreter frames. Part of this is relocking
objects with eliminated
locking. New with the enhancement is that we do this also just before
object references are acquired
through JVMTI. In this case we deoptimize also the owning compiled
frame C and we register
deoptimized objects as deferred updates. When control returns to C
it
gets deoptimized, we notice
that objects are already deoptimized (reallocated and relocked), so
we
don't do it again (relocking
twice would be incorrect of course). Deferred updates are copied into
the new interpreter frames.
Problem: relocking is not possible if the target thread T is waiting
on the monitor that needs to be
relocked. This happens only with non-local objects with
EliminateNestedLocks. Instead relocking is
deferred until T owns the monitor again. This is what the piece of
code above does.
Sorry I need some more detail here. How can you wait() on an object
monitor if the object allocation and/or locking was optimised away?
And
what is a "non-local object" in this context? Isn't EA restricted to
thread-confined objects?
Is it just that some of the locking gets optimized away e.g.
synchronised(obj) {
synchronised(obj) {
synchronised(obj) {
obj.wait();
}
}
}
If this is reduced to a form as-if it were a single lock of the monitor
(due to EA) and the wait() triggers a JVM TI event which leads to the
escape of "obj" then we need to reconstruct the true lock state, and so
when the wait() internally unblocks and reacquires the monitor it has to
set the true recursion count to 3, not the 1 that it appeared to be when
wait() was initially called. Is that the scenario?
If so I find this truly awful. Anyone using wait() in a realistic form
requires a notification and so the object cannot be thread confined. In
which case I would strongly argue that upon hitting the wait() the
deopt
should occur unconditionally and so the lock state is correct before we
wait and so we don't need to mess with the recursion count internally
when we reacquire the monitor.
> which I don't like the sound of at all when it comes to
ObjectMonitor
> state. So I'd like to understand in detail exactly what is going
on here
> and why. This is a very intrusive change that seems to badly
break
> encapsulation and impacts future changes to ObjectMonitor that
are under
> investigation.
I would not regard this as breaking encapsulation. Certainly not badly.
I've added a property relock_count_after_wait to JavaThread. The
property is well
encapsulated. Future ObjectMonitor implementations have to deal
with
recursion too. They are free in
choosing a way to do that as long as that property is taken into
account. This is hardly a
limitation.
I do think this badly breaks encapsulation as you have to add a callout
from the guts of the ObjectMonitor code to reach into the thread to
get
this lock count adjustment. I understand why you have had to do this
but
I would much rather see a change to the EA optimisation strategy so
that
this is not needed.
Note also that the property is a straight forward extension of the
existing concept of deferred
local updates. It is embedded into the structure holding them. So not
even the footprint of a
JavaThread is enlarged if no deferred updates are generated.
> ---
>
> src/hotspot/share/runtime/thread.cpp
>
> Can you please explain why
JavaThread::wait_for_object_deoptimization
> has to be handcrafted in this way rather than using proper
transitions.
>
I wrote wait_for_object_deoptimization taking
JavaThread::java_suspend_self_with_safepoint_check
as template. So in short: for the same reasons :)
Threads reach both methods as part of thread state transitions,
therefore special handling is
required to change thread state on top of ongoing transitions.
> We got rid of "deopt suspend" some time ago and it is disturbing
to see
> it being added back (effectively). This seems like it may be
something
> that handshakes could be used for.
Deopt suspend used to be something rather different with a similar
name[1]. It is not being added back.
I stand corrected. Despite comments in the code to the contrary
deopt_suspend didn't actually cause a self-suspend. I was doing a lot of
cleanup in this area 13 years ago :)
I'm actually duplicating the existing external suspend mechanism,
because a thread can be suspended
at most once. And hey, and don't like that either! But it seems not
unlikely that the duplicate can
be removed together with the original and the new type of
handshakes
that will be used for
thread suspend can be used for object deoptimization too. See
today's
discussion in JDK-8227745 [2].
I hope that discussion bears some fruit, at the moment it seems not to
be possible to use handshakes here. :(
The external suspend mechanism is a royal pain in the proverbial that
we
have to carefully live with. The idea that we're duplicating that for
use in another fringe area of functionality does not thrill me at all.
To be clear, I understand the problem that exists and that you wish to
solve, but for the runtime parts I balk at the complexity cost of
solving it.
Thanks,
David
-----
Thanks, Richard.
[1] Deopt suspend was something like an async. handshake for
architectures with register windows,
where patching the return pc for deoptimization of a compiled
frame was racy if the owner thread
was in native code. Instead a "deopt" suspend flag was set on
which the thread patched its own
frame upon return from native. So no thread was suspended. It
got
its name only from the name of
the flags.
[2] Discussion about using handshakes to sync. with the target thread:
https://bugs.openjdk.java.net/browse/JDK-
8227745?focusedCommentId=14306727&page=com.atlassian.jira.plugin.syst
e
m.issuetabpanels:comment-tabpanel#comment-14306727
-----Original Message-----
From: David Holmes <david.hol...@oracle.com>
Sent: Freitag, 13. Dezember 2019 00:56
To: Reingruber, Richard <richard.reingru...@sap.com>;
serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better
Performance in the Presence of JVMTI Agents
Hi Richard,
Some further queries/concerns:
src/hotspot/share/runtime/objectMonitor.cpp
Can you please explain the changes to ObjectMonitor::wait:
! _recursions = save // restore the old recursion count
! + jt->get_and_reset_relock_count_after_wait(); //
increased by the deferred relock count
what is the "deferred relock count"? I gather it relates to
"The code was extended to be able to deoptimize objects of a frame
that
is not the top frame and to let another thread than the owning thread
do
it."
which I don't like the sound of at all when it comes to ObjectMonitor
state. So I'd like to understand in detail exactly what is going on here
and why. This is a very intrusive change that seems to badly break
encapsulation and impacts future changes to ObjectMonitor that are
under
investigation.
---
src/hotspot/share/runtime/thread.cpp
Can you please explain why
JavaThread::wait_for_object_deoptimization
has to be handcrafted in this way rather than using proper transitions.
We got rid of "deopt suspend" some time ago and it is disturbing to
see
it being added back (effectively). This seems like it may be something
that handshakes could be used for.
Thanks,
David
-----
On 12/12/2019 7:02 am, David Holmes wrote:
On 12/12/2019 1:07 am, Reingruber, Richard wrote:
Hi David,
> Most of the details here are in areas I can comment on in
detail,
but I
> did take an initial general look at things.
Thanks for taking the time!
Apologies the above should read:
"Most of the details here are in areas I *can't* comment on in detail
..."
David
> The only thing that jumped out at me is that I think the
> DeoptimizeObjectsALotThread should be a hidden thread.
>
> + bool is_hidden_from_external_view() const { return true; }
Yes, it should. Will add the method like above.
> Also I don't see any testing of the
DeoptimizeObjectsALotThread.
Without
> active testing this will just bit-rot.
DeoptimizeObjectsALot is meant for stress testing with a larger
workload. I will add a minimal test
to keep it fresh.
> Also on the tests I don't understand your @requires clause:
>
> @requires ((vm.compMode != "Xcomp") &
vm.compiler2.enabled
&
> (vm.opt.TieredCompilation != true))
>
> This seems to require that TieredCompilation is disabled, but
tiered is
> our normal mode of operation. ??
>
I removed the clause. I guess I wanted to target the tests towards
the
code they are supposed to
test, and it's easier to analyze failures w/o tiered compilation and
with just one compiler thread.
Additionally I will make use of
compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests.
Thanks,
Richard.
-----Original Message-----
From: David Holmes <david.hol...@oracle.com>
Sent: Mittwoch, 11. Dezember 2019 08:03
To: Reingruber, Richard <richard.reingru...@sap.com>;
serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better
Performance in the Presence of JVMTI Agents
Hi Richard,
On 11/12/2019 7:45 am, Reingruber, Richard wrote:
Hi,
I would like to get reviews please for
http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/
Corresponding RFE:
https://bugs.openjdk.java.net/browse/JDK-8227745
Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915
And potentially https://bugs.openjdk.java.net/browse/JDK-
8214584 [1]
Vladimir Kozlov kindly put webrev.3 through tier1-8 testing
without
issues (thanks!). In addition the
change is being tested at SAP since I posted the first RFR some
months ago.
The intention of this enhancement is to benefit performance wise
from
escape analysis even if JVMTI
agents request capabilities that allow them to access local variable
values. E.g. if you start-up
with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
then
escape analysis is disabled right
from the beginning, well before a debugger attaches -- if ever one
should do so. With the
enhancement, escape analysis will remain enabled until and after
a
debugger attaches. EA based
optimizations are reverted just before an agent acquires the
reference to an object. In the JBS item
you'll find more details.
Most of the details here are in areas I can comment on in detail, but
I
did take an initial general look at things.
The only thing that jumped out at me is that I think the
DeoptimizeObjectsALotThread should be a hidden thread.
+ bool is_hidden_from_external_view() const { return true; }
Also I don't see any testing of the DeoptimizeObjectsALotThread.
Without
active testing this will just bit-rot.
Also on the tests I don't understand your @requires clause:
@requires ((vm.compMode != "Xcomp") &
vm.compiler2.enabled &
(vm.opt.TieredCompilation != true))
This seems to require that TieredCompilation is disabled, but tiered
is
our normal mode of operation. ??
Thanks,
David
Thanks,
Richard.
[1] Experimental fix for JDK-8214584 based on JDK-8227745
http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.pa
tc
h
