Hi Richard,
On 28/04/2020 12:09 am, Reingruber, Richard wrote:
Hi David,
Not a review but some general commentary ...
That's welcome.
Having had to take an even closer look now I have a review comment too :)
src/hotspot/share/prims/jvmtiThreadState.cpp
void JvmtiThreadState::invalidate_cur_stack_depth() {
! assert(SafepointSynchronize::is_at_safepoint() ||
! (Thread::current()->is_VM_thread() &&
get_thread()->is_vmthread_processing_handshake()) ||
(JavaThread *)Thread::current() == get_thread(),
"must be current thread or at safepoint");
The message needs updating to include handshakes.
More below ...
On 25/04/2020 2:08 am, Reingruber, Richard wrote:
Hi Yasumasa, Patricio,
I will send review request to replace VM_SetFramePop to handshake in early next
week in JDK-8242427.
Does it help you? I think it gives you to remove workaround.
I think it would not help that much. Note that when replacing VM_SetFramePop
with a direct handshake
you could not just execute VM_EnterInterpOnlyMode as a nested vm operation [1].
So you would have to
change/replace VM_EnterInterpOnlyMode and I would have to adapt to these
changes.
Thanks for your information.
I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and
vmTestbase/nsk/jvmti/NotifyFramePop.
I will modify and will test it after yours.
Thanks :)
Also my first impression was that it won't be that easy from a synchronization
point of view to
replace VM_SetFramePop with a direct handshake. E.g. VM_SetFramePop::doit()
indirectly calls
JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop
fpop) where
JvmtiThreadState_lock is acquired with safepoint check, if not at safepoint.
It's not directly clear
to me, how this has to be handled.
I think JvmtiEventController::set_frame_pop() should hold JvmtiThreadState_lock
because it affects other JVMTI operation especially FramePop event.
Yes. To me it is unclear what synchronization is necessary, if it is called
during a handshake. And
also I'm unsure if a thread should do safepoint checks while executing a
handshake.
I'm growing increasingly concerned that use of direct handshakes to
replace VM operations needs a much greater examination for correctness
than might initially be thought. I see a number of issues:
I agree. I'll address your concerns in the context of this review thread for
JDK-8238585 below.
In addition I would suggest to take the general part of the discussion to a
dedicated thread or to
the review thread for JDK-8242427. I would like to keep this thread closer to
its subject.
I will focus on the issues in the context of this particular change
then, though the issues themselves are applicable to all handshake
situations (and more so with direct handshakes). This is mostly just
discussion.
First, the VMThread executes (most) VM operations with a clean stack in
a clean state, so it has lots of room to work. If we now execute the
same logic in a JavaThread then we risk hitting stackoverflows if
nothing else. But we are also now executing code in a JavaThread and so
we have to be sure that code is not going to act differently (in a bad
way) if executed by a JavaThread rather than the VMThread. For example,
may it be possible that if executing in the VMThread we defer some
activity that might require execution of Java code, or else hand it off
to one of the service threads? If we execute that code directly in the
current JavaThread instead we may not be in a valid state (e.g. consider
re-entrancy to various subsystems that is not allowed).
It is not too complex, what EnterInterpOnlyModeClosure::do_thread() is doing. I
already added a
paragraph to the JBS-Item [1] explaining why the direct handshake is sufficient
from a
synchronization point of view.
Just to be clear, your proposed change is not using a direct handshake.
Furthermore the stack is walked and the return pc of compiled frames is
replaced with the address of
the deopt handler.
I can't see why this cannot be done with a direct handshake. Something very
similar is already done
in JavaThread::deoptimize_marked_methods() which is executed as part of an
ordinary handshake.
Note that existing non-direct handshakes may also have issues that not
have been fully investigated.
The demand on stack-space should be very modest. I would not expect a higher
risk for stackoverflow.
For the target thread if you use more stack than would be used stopping
at a safepoint then you are at risk. For the thread initiating the
direct handshake if you use more stack than would be used enqueuing a VM
operation, then you are at risk. As we have not quantified these
numbers, nor have any easy way to establish the stack use of the actual
code to be executed, we're really just hoping for the best. This is a
general problem with handshakes that needs to be investigated more
deeply. As a simple, general, example just imagine if the code involves
logging that might utilise an on-stack buffer.
Second, we have this question mark over what happens if the operation
hits further safepoint or handshake polls/checks? Are there constraints
on what is allowed here? How can we recognise this problem may exist and
so deal with it?
The thread in EnterInterpOnlyModeClosure::do_thread() can't become
safepoint/handshake safe. I
tested locally test/hotspot/jtreg:vmTestbase_nsk_jvmti with a
NoSafepointVerifier.
That's good to hear but such tests are not exhaustive, they will detect
if you do reach a safepoint/handshake but they can't prove that you
cannot reach one. What you have done is necessary but may not be
sufficient. Plus you didn't actually add the NSV to the code - is there
a reason we can't actually keep it in do_thread? (I'm not sure if the
NSV also acts as a NoHandshakeVerifier?)
Third, while we are generally considering what appear to be
single-thread operations, which should be amenable to a direct
handshake, we also have to be careful that some of the code involved
doesn't already expect/assume we are at a safepoint - e.g. a VM op may
not need to take a lock where a direct handshake might!
See again my arguments in the JBS item [1].
Yes I see the reasoning and that is good. My point is a general one as
it may not be obvious when such assumptions exist in the current code.
Thanks,
David
Thanks,
Richard.
[1] https://bugs.openjdk.java.net/browse/JDK-8238585
-----Original Message-----
From: David Holmes <david.hol...@oracle.com>
Sent: Montag, 27. April 2020 07:16
To: Reingruber, Richard <richard.reingru...@sap.com>; Yasumasa Suenaga
<suen...@oss.nttdata.com>; Patricio Chilano <patricio.chilano.ma...@oracle.com>;
serguei.spit...@oracle.com; Vladimir Ivanov <vladimir.x.iva...@oracle.com>;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
methods on stack not_entrant
Hi all,
Not a review but some general commentary ...
On 25/04/2020 2:08 am, Reingruber, Richard wrote:
Hi Yasumasa, Patricio,
I will send review request to replace VM_SetFramePop to handshake in early next
week in JDK-8242427.
Does it help you? I think it gives you to remove workaround.
I think it would not help that much. Note that when replacing VM_SetFramePop
with a direct handshake
you could not just execute VM_EnterInterpOnlyMode as a nested vm operation [1].
So you would have to
change/replace VM_EnterInterpOnlyMode and I would have to adapt to these
changes.
Thanks for your information.
I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and
vmTestbase/nsk/jvmti/NotifyFramePop.
I will modify and will test it after yours.
Thanks :)
Also my first impression was that it won't be that easy from a synchronization
point of view to
replace VM_SetFramePop with a direct handshake. E.g. VM_SetFramePop::doit()
indirectly calls
JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop
fpop) where
JvmtiThreadState_lock is acquired with safepoint check, if not at safepoint.
It's not directly clear
to me, how this has to be handled.
I think JvmtiEventController::set_frame_pop() should hold JvmtiThreadState_lock
because it affects other JVMTI operation especially FramePop event.
Yes. To me it is unclear what synchronization is necessary, if it is called
during a handshake. And
also I'm unsure if a thread should do safepoint checks while executing a
handshake.
I'm growing increasingly concerned that use of direct handshakes to
replace VM operations needs a much greater examination for correctness
than might initially be thought. I see a number of issues:
First, the VMThread executes (most) VM operations with a clean stack in
a clean state, so it has lots of room to work. If we now execute the
same logic in a JavaThread then we risk hitting stackoverflows if
nothing else. But we are also now executing code in a JavaThread and so
we have to be sure that code is not going to act differently (in a bad
way) if executed by a JavaThread rather than the VMThread. For example,
may it be possible that if executing in the VMThread we defer some
activity that might require execution of Java code, or else hand it off
to one of the service threads? If we execute that code directly in the
current JavaThread instead we may not be in a valid state (e.g. consider
re-entrancy to various subsystems that is not allowed).
Second, we have this question mark over what happens if the operation
hits further safepoint or handshake polls/checks? Are there constraints
on what is allowed here? How can we recognise this problem may exist and
so deal with it?
Third, while we are generally considering what appear to be
single-thread operations, which should be amenable to a direct
handshake, we also have to be careful that some of the code involved
doesn't already expect/assume we are at a safepoint - e.g. a VM op may
not need to take a lock where a direct handshake might!
Cheers,
David
-----
@Patricio, coming back to my question [1]:
In the example you gave in your answer [2]: the java thread would execute a vm
operation during a
direct handshake operation, while the VMThread is actually in the middle of a
VM_HandshakeAllThreads
operation, waiting to handshake the same handshakee: why can't the VMThread
just proceed? The
handshakee would be safepoint safe, wouldn't it?
Thanks, Richard.
[1]
https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301677&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301677
[2]
https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301763&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301763
-----Original Message-----
From: Yasumasa Suenaga <suen...@oss.nttdata.com>
Sent: Freitag, 24. April 2020 17:23
To: Reingruber, Richard <richard.reingru...@sap.com>; Patricio Chilano
<patricio.chilano.ma...@oracle.com>; serguei.spit...@oracle.com; Vladimir Ivanov
<vladimir.x.iva...@oracle.com>; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
methods on stack not_entrant
Hi Richard,
On 2020/04/24 23:44, Reingruber, Richard wrote:
Hi Yasumasa,
I will send review request to replace VM_SetFramePop to handshake in early next
week in JDK-8242427.
Does it help you? I think it gives you to remove workaround.
I think it would not help that much. Note that when replacing VM_SetFramePop
with a direct handshake
you could not just execute VM_EnterInterpOnlyMode as a nested vm operation [1].
So you would have to
change/replace VM_EnterInterpOnlyMode and I would have to adapt to these
changes.
Thanks for your information.
I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and
vmTestbase/nsk/jvmti/NotifyFramePop.
I will modify and will test it after yours.
Also my first impression was that it won't be that easy from a synchronization
point of view to
replace VM_SetFramePop with a direct handshake. E.g. VM_SetFramePop::doit()
indirectly calls
JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop
fpop) where
JvmtiThreadState_lock is acquired with safepoint check, if not at safepoint.
It's not directly clear
to me, how this has to be handled.
I think JvmtiEventController::set_frame_pop() should hold JvmtiThreadState_lock
because it affects other JVMTI operation especially FramePop event.
Thanks,
Yasumasa
So it appears to me that it would be easier to push JDK-8242427 after this
(JDK-8238585).
(The patch is available, but I want to see the result of PIT in this weekend
whether JDK-8242425 works fine.)
Would be interesting to see how you handled the issues above :)
Thanks, Richard.
[1] See question in comment
https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14302030&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14302030
-----Original Message-----
From: Yasumasa Suenaga <suen...@oss.nttdata.com>
Sent: Freitag, 24. April 2020 13:34
To: Reingruber, Richard <richard.reingru...@sap.com>; Patricio Chilano
<patricio.chilano.ma...@oracle.com>; serguei.spit...@oracle.com; Vladimir Ivanov
<vladimir.x.iva...@oracle.com>; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
methods on stack not_entrant
Hi Richard,
I will send review request to replace VM_SetFramePop to handshake in early next
week in JDK-8242427.
Does it help you? I think it gives you to remove workaround.
(The patch is available, but I want to see the result of PIT in this weekend
whether JDK-8242425 works fine.)
Thanks,
Yasumasa
On 2020/04/24 17:18, Reingruber, Richard wrote:
Hi Patricio, Vladimir, and Serguei,
now that direct handshakes are available, I've updated the patch to make use of
them.
In addition I have done some clean-up changes I missed in the first webrev.
Finally I have implemented the workaround suggested by Patricio to avoid
nesting the handshake
into the vm operation VM_SetFramePop [1]
Kindly review again:
Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/
Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/
I updated the JBS item explaining why the vm operation VM_EnterInterpOnlyMode
can be replaced with a
direct handshake:
JBS: https://bugs.openjdk.java.net/browse/JDK-8238585
Testing:
* JCK and JTREG tests, also in Xcomp mode with fastdebug and release builds on
all platforms.
* Submit-repo: mach5-one-rrich-JDK-8238585-20200423-1436-10441737
Thanks,
Richard.
[1] An assertion in Handshake::execute_direct() fails, if called be VMThread,
because it is no JavaThread.
-----Original Message-----
From: hotspot-dev <hotspot-dev-boun...@openjdk.java.net> On Behalf Of
Reingruber, Richard
Sent: Freitag, 14. Februar 2020 19:47
To: Patricio Chilano <patricio.chilano.ma...@oracle.com>;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@openjdk.java.net
Subject: RE: RFR(S) 8238585: Use handshake for
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
methods on stack not_entrant
Hi Patricio,
> > I'm really glad you noticed the problematic nesting. This seems to be
a general issue: currently a
> > handshake cannot be nested in a vm operation. Maybe it should be
asserted in the
> > Handshake::execute() methods that they are not called by the vm
thread evaluating a vm operation?
> >
> > > Alternatively I think you could do something similar to what we
do in
> > > Deoptimization::deoptimize_all_marked():
> > >
> > > EnterInterpOnlyModeClosure hs;
> > > if (SafepointSynchronize::is_at_safepoint()) {
> > > hs.do_thread(state->get_thread());
> > > } else {
> > > Handshake::execute(&hs, state->get_thread());
> > > }
> > > (you could pass “EnterInterpOnlyModeClosure” directly to the
> > > HandshakeClosure() constructor)
> >
> > Maybe this could be used also in the Handshake::execute() methods as
general solution?
> Right, we could also do that. Avoiding to clear the polling page in
> HandshakeState::clear_handshake() should be enough to fix this issue and
> execute a handshake inside a safepoint, but adding that "if" statement
> in Hanshake::execute() sounds good to avoid all the extra code that we
> go through when executing a handshake. I filed 8239084 to make that
change.
Thanks for taking care of this and creating the RFE.
>
> > > I don’t know JVMTI code so I’m not sure if
VM_EnterInterpOnlyMode is
> > > always called in a nested operation or just sometimes.
> >
> > At least one execution path without vm operation exists:
> >
> >
JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *) : void
> >
JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState *) : jlong
> > JvmtiEventControllerPrivate::recompute_enabled() : void
> > JvmtiEventControllerPrivate::change_field_watch(jvmtiEvent,
bool) : void (2 matches)
> > JvmtiEventController::change_field_watch(jvmtiEvent, bool)
: void
> > JvmtiEnv::SetFieldAccessWatch(fieldDescriptor *) :
jvmtiError
> > jvmti_SetFieldAccessWatch(jvmtiEnv *, jclass,
jfieldID) : jvmtiError
> >
> > I tend to revert back to VM_EnterInterpOnlyMode as it wasn't my main
intent to replace it with a
> > handshake, but to avoid making the compiled methods on stack
not_entrant.... unless I'm further
> > encouraged to do it with a handshake :)
> Ah! I think you can still do it with a handshake with the
> Deoptimization::deoptimize_all_marked() like solution. I can change the
> if-else statement with just the Handshake::execute() call in 8239084.
> But up to you. : )
Well, I think that's enough encouragement :)
I'll wait for 8239084 and try then again.
(no urgency and all)
Thanks,
Richard.
-----Original Message-----
From: Patricio Chilano <patricio.chilano.ma...@oracle.com>
Sent: Freitag, 14. Februar 2020 15:54
To: Reingruber, Richard <richard.reingru...@sap.com>;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
methods on stack not_entrant
Hi Richard,
On 2/14/20 9:58 AM, Reingruber, Richard wrote:
Hi Patricio,
thanks for having a look.
> I’m only commenting on the handshake changes.
> I see that operation VM_EnterInterpOnlyMode can be called inside
> operation VM_SetFramePop which also allows nested operations. Here is a
> comment in VM_SetFramePop definition:
>
> // Nested operation must be allowed for the VM_EnterInterpOnlyMode
that is
> // called from the
JvmtiEventControllerPrivate::recompute_thread_enabled.
>
> So if we change VM_EnterInterpOnlyMode to be a handshake, then now we
> could have a handshake inside a safepoint operation. The issue I see
> there is that at the end of the handshake the polling page of the
target
> thread could be disarmed. So if the target thread happens to be in a
> blocked state just transiently and wakes up then it will not stop for
> the ongoing safepoint. Maybe I can file an RFE to assert that the
> polling page is armed at the beginning of disarm_safepoint().
I'm really glad you noticed the problematic nesting. This seems to be a general
issue: currently a
handshake cannot be nested in a vm operation. Maybe it should be asserted in the
Handshake::execute() methods that they are not called by the vm thread
evaluating a vm operation?
> Alternatively I think you could do something similar to what we do in
> Deoptimization::deoptimize_all_marked():
>
> EnterInterpOnlyModeClosure hs;
> if (SafepointSynchronize::is_at_safepoint()) {
> hs.do_thread(state->get_thread());
> } else {
> Handshake::execute(&hs, state->get_thread());
> }
> (you could pass “EnterInterpOnlyModeClosure” directly to the
> HandshakeClosure() constructor)
Maybe this could be used also in the Handshake::execute() methods as general
solution?
Right, we could also do that. Avoiding to clear the polling page in
HandshakeState::clear_handshake() should be enough to fix this issue and
execute a handshake inside a safepoint, but adding that "if" statement
in Hanshake::execute() sounds good to avoid all the extra code that we
go through when executing a handshake. I filed 8239084 to make that change.
> I don’t know JVMTI code so I’m not sure if VM_EnterInterpOnlyMode is
> always called in a nested operation or just sometimes.
At least one execution path without vm operation exists:
JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *)
: void
JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState
*) : jlong
JvmtiEventControllerPrivate::recompute_enabled() : void
JvmtiEventControllerPrivate::change_field_watch(jvmtiEvent, bool)
: void (2 matches)
JvmtiEventController::change_field_watch(jvmtiEvent, bool) : void
JvmtiEnv::SetFieldAccessWatch(fieldDescriptor *) : jvmtiError
jvmti_SetFieldAccessWatch(jvmtiEnv *, jclass, jfieldID) :
jvmtiError
I tend to revert back to VM_EnterInterpOnlyMode as it wasn't my main intent to
replace it with a
handshake, but to avoid making the compiled methods on stack not_entrant....
unless I'm further
encouraged to do it with a handshake :)
Ah! I think you can still do it with a handshake with the
Deoptimization::deoptimize_all_marked() like solution. I can change the
if-else statement with just the Handshake::execute() call in 8239084.
But up to you. : )
Thanks,
Patricio
Thanks again,
Richard.
-----Original Message-----
From: Patricio Chilano <patricio.chilano.ma...@oracle.com>
Sent: Donnerstag, 13. Februar 2020 18:47
To: Reingruber, Richard <richard.reingru...@sap.com>;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
methods on stack not_entrant
Hi Richard,
I’m only commenting on the handshake changes.
I see that operation VM_EnterInterpOnlyMode can be called inside
operation VM_SetFramePop which also allows nested operations. Here is a
comment in VM_SetFramePop definition:
// Nested operation must be allowed for the VM_EnterInterpOnlyMode that is
// called from the JvmtiEventControllerPrivate::recompute_thread_enabled.
So if we change VM_EnterInterpOnlyMode to be a handshake, then now we
could have a handshake inside a safepoint operation. The issue I see
there is that at the end of the handshake the polling page of the target
thread could be disarmed. So if the target thread happens to be in a
blocked state just transiently and wakes up then it will not stop for
the ongoing safepoint. Maybe I can file an RFE to assert that the
polling page is armed at the beginning of disarm_safepoint().
I think one option could be to remove
SafepointMechanism::disarm_if_needed() in
HandshakeState::clear_handshake() and let each JavaThread disarm itself
for the handshake case.
Alternatively I think you could do something similar to what we do in
Deoptimization::deoptimize_all_marked():
EnterInterpOnlyModeClosure hs;
if (SafepointSynchronize::is_at_safepoint()) {
hs.do_thread(state->get_thread());
} else {
Handshake::execute(&hs, state->get_thread());
}
(you could pass “EnterInterpOnlyModeClosure” directly to the
HandshakeClosure() constructor)
I don’t know JVMTI code so I’m not sure if VM_EnterInterpOnlyMode is
always called in a nested operation or just sometimes.
Thanks,
Patricio
On 2/12/20 7:23 AM, Reingruber, Richard wrote:
// Repost including hotspot runtime and gc lists.
// Dean Long suggested to do so, because the enhancement replaces a vm operation
// with a handshake.
// Original thread:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-February/030359.html
Hi,
could I please get reviews for this small enhancement in hotspot's jvmti
implementation:
Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8238585
The change avoids making all compiled methods on stack not_entrant when
switching a java thread to
interpreter only execution for jvmti purposes. It is sufficient to deoptimize
the compiled frames on stack.
Additionally a handshake is used instead of a vm operation to walk the stack
and do the deoptimizations.
Testing: JCK and JTREG tests, also in Xcomp mode with fastdebug and release
builds on all platforms.
Thanks, Richard.
See also my question if anyone knows a reason for making the compiled methods
not_entrant:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030339.html
