Hi Yasumasa,
On 25/06/2020 6:24 pm, Yasumasa Suenaga wrote:
Hi David,
Thanks for your comment!
On 2020/06/25 14:17, David Holmes wrote:
Hi Yasumasa,
Thanks for tackling this. I've had an initial look at it and have a
few concerns.
On 24/06/2020 4:50 pm, Yasumasa Suenaga wrote:
Hi all,
Please review this change:
JBS: https://bugs.openjdk.java.net/browse/JDK-8242428
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/
Some typos:
invaliant -> invariant
directry -> directly
I will fix them.
This change replace following VM operations to direct handshake.
- VM_GetFrameCount (GetFrameCount())
- VM_GetFrameLocation (GetFrameLocation())
- VM_GetThreadListStackTraces (GetThreadListStackTrace())
- VM_GetCurrentLocation
It would have been better to split these out into separate changes. I
am finding it very hard to track through the webrev and try to compare
the old safepoint based operation with the new direct handshake
approach, to check they are functionally equivalent.
I will separate them as following. What do you think?
If you are ok, I will update JBS.
- Thread operations
- VM_GetThreadListStackTraces (GetThreadListStackTrace())
- VM_GetStackTrace(GetStackTrace()) <- I missed it to describe in
previous mail, sorry.
- Frame operations
- VM_GetFrameCount (GetFrameCount())
- VM_GetFrameLocation (GetFrameLocation())
- VM_GetCurrentLocation
I will start to work when they are separated.
If the frame operations are each small enough that will help.
You are not checking the return value of Handshake::execute_direct and
so are missing the possibility that the target thread has terminated
before you got to do the operation on it. It isn't clear to me under
what other circumstances execute_direct can also return false.
I will add it. According to Handshake::execute_direct() and
HandshakeOperation::do_handshake(), it seems to return false if the
target thread has terminated as you said.
Yes, but also if the handshake is not executed - but I don't know under
what conditions that can occur.
You don't seem to have these checks anymore in some places:
&& !_java_thread->is_exiting() && _java_thread->threadObj() != NULL)
why not?
I thought the thread which enters handshake is always alive and it has
threadObj.
As far as I can see we can still engage in a handshake with a thread
after it has marked itself as exiting.
The threadObj() can only be null while a thread is attaching, which
means it would have to checked in the general case, but for these JVM TI
operations if we already have a jthread reference to the target thread
then it must be beyond that point. Mind you that same logic applies to
the existing code so ...
I will recover their conditions.
(I also should recover them for GetOwnedMonitorInfoClosure and
GetCurrentContendedMonitorClosure - I removed them in JDK-8242425)
I think so - and we need to check the return value of execute_direct to
determine when to report JVMTI_ERROR_THREAD_NOT_ALIVE.
It is not clear that all the code that previously could execute at a
safepoint, due to being called from a VM_Operation, is still
executable at a safepoint e.g. JvmtiThreadState::count_frames()
GetThreadListStackTrace() uses direct handshake if thread count == 1.
In other case (thread count > 1), it would be performed as VM
operation (VM_GetThreadListStackTraces).
This introduces a large chunk of duplicated code for the frame fill in
and final allocation. Can you not reuse the existing logic that does
this - and in the process do away with the the use of
_needs_thread_state? I really wanted to see simpler code after this
conversion.
I'm also wondering whether we can hide all this logic in the closure,
as was done with the VM_Operation i.e.
*stack_info_ptr = op.stack_info();
I will try to refactor this change.
Caller of VM_GetCurrentLocation
(JvmtiEnvThreadState::reset_current_location()) might be called at
safepoint. So I added safepoint check in its caller.
I could not figure out what you were referring to here.
I guess following callpath is available:
VM_GetCurrentLocation
JvmtiEnvThreadState::reset_current_location()
JvmtiEventControllerPrivate::recompute_env_thread_enabled()
JvmtiEventControllerPrivate::recompute_thread_enabled()
JvmtiEventControllerPrivate::set_frame_pop()
JvmtiEventController::set_frame_pop()
JvmtiEnvThreadState::set_frame_pop()
VM_SetFramePop::doit()
However, VM_SetFramePop seems not to allow nested VM operations.
It is the outer operation that has to allow nesting but
VM_GetCurrentLocation doesn't allow it either. So if this path is
possible then something is broken.
Cheers,
David
This change has been tested in serviceability/jvmti
serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi
vmTestbase/ns
k/jdwp.
Just a general comment on testing for these conversions to direct
handshakes. We have no control over whether the handshake gets
executed in the original thread or the target thread, so for all we
know all our testing could be executing only one of the cases. This
concerns me but I am not yet sure what to do about it.
Thanks,
David
-----
Also I tested it on submit repo, then it has execution error
(mach5-one-ysuenaga-JDK-8242428-20200624-0054-12034717) due to
dependency error. So I think it does not occur by this change.
Thanks,
Yasumasa
