On 7/22/20 9:25 PM, David Holmes wrote:
Hi Dan,
I just want to respond to one aspect of your response ...
On 23/07/2020 11:04 am, Daniel D. Daugherty wrote:
On 7/22/20 10:38 AM, Yasumasa Suenaga wrote:
Hi all,
Please review this change:
JBS: https://bugs.openjdk.java.net/browse/JDK-8248362
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8248362/webrev.00/
src/hotspot/share/prims/jvmtiEnv.cpp
L1755 // JVMTI get java stack frame location at safepoint.
You missed updating this one. Perhaps:
// JVMTI get java stack frame location via direct
handshake.
src/hotspot/share/prims/jvmtiEnvBase.cpp
L1563: JavaThread* jt = _state->get_thread();
L1564: assert(target == jt, "just checking");
This code is different than the other closures. It might be
worth
a comment to explain why. I don't remember why
VM_GetFrameCount had
to use the JvmtiThreadState to fetch the JavaThread. Serguei
might
remember.
It could be that we don't need the _state field anymore
because of
the way that handshakes work (and provide the JavaThread* to
the
do_thread() function). Your choice on whether to deal with
that as
part of this fix or following with another RFE.
Update: Uggg.... the get_frame_count() function takes
JvmtiThreadState
as a parameter. This is very much entangled... sigh... we
should
definitely look at untangling this in another RFE...
old L1565: ThreadsListHandle tlh;
new L1565: if (!jt->is_exiting() && jt->threadObj() != NULL) {
Please consider add this comment above L1565:
// ThreadsListHandle is not needed due to direct
handshake.
Yes, I see that previous closures were added without that
comment,
but when I see "if (!jt->is_exiting() && jt->threadObj() !=
NULL)"
I immediately wonder where the ThreadsListHandle is...
Please consider
adding the comment to the others also.
I understand why, when looking at this change, you would like the
comment, but outside of the webrev the reference to ThreadsListHandle
doesn't really have any context. We need a TLH anywhere there is a
risk the target thread might exit while we're interacting with it, but
that can't happen in the context of a direct handshake operation with
the target thread because the TLH exists in our caller. This is the
new normal for code that executes as part of a direct handshake.
Agreed. I have to adjust to the "new normal".
Dan
David
-----
old L1574: ThreadsListHandle tlh;
new L1574: if (!jt->is_exiting() && jt->threadObj() != NULL) {
Please consider add this comment above L1574:
// ThreadsListHandle is not needed due to direct
handshake.
src/hotspot/share/prims/jvmtiEnvBase.hpp
L580: // HandshakeClosure to frame location.
typo - s/to frame/to get frame/
src/hotspot/share/prims/jvmtiThreadState.cpp
No comments on the changes.
For the above comments about VM_GetFrameCount, understanding why
JvmtiThreadState::count_frames() has to exist in JvmtiThreadState
will go along way towards untangling the mess.
src/hotspot/share/runtime/vmOperations.hpp
No comments.
Thumbs up. I only have nits above. If you choose to make the above
changes, I do not need to see another webrev.
Dan
Migrate JVMTI frame operations to Thread-Local Handshakes from VM
Operations.
- VM_GetFrameCount
- VM_GetFrameLocation
They affects both GetFrameCount() and GetFrameLocation() JVMTI
functions.
This change has passed all tests on submit repo
(mach5-one-ysuenaga-JDK-8248362-20200722-1249-12859056), and
vmTestbase/nsk/{jdi,jdw,jvmti}, serviceability/{jdwp,jvmti} .
Thanks,
Yasumasa
