Hi Robbin,
On 30/06/2020 11:03 pm, Robbin Ehn wrote:
Hi Yasumasa,
On 2020-06-30 14:35, Yasumasa Suenaga wrote:
Hi Robbin,
We decided to separate thread operation and frame operation.
I've posted review request for thread operation. Could you review it?
Yes I know but I'm soon off for vacation and you have not sent them all
out and due to the nature of my comments replied here.
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
We can share HandshakeClosure for GetStackTrace() to
GetFrameLocation() as you said.
However I wonder why it is not so now.
I guess GetStackTrace() would give some overhead (e.g. memory
allocation for jvmtiFrameInfo) if we use it for frame location.
In case of GetFrameLocation we only need one and it's only a dozen bytes
big I wouldn't count that as an overhead in this case.
If you are really paranoid you can stack allocated it and pass it in.
I thought we should replace VM operation to HandshakeClosure one by one.
I will merge these operations as possible in JDK-8248362 if we should do.
If you make the first one generic enough or evolve it you can convert VM
op to your new function instead, thus doing one by one.
So in http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/ I
really think:
1575 JvmtiEnv::GetThreadListStackTraces(jint thread_count, const
jthread* thread_list, jint max_frame_count, jvmtiStackInfo**
stack_info_ptr) {
.....
1578 if (thread_count == 1) {
===> // This case should be the same as JvmtiEnv::GetStackTrace
1592 } else {
It effectively is, but it can't be exactly the same because the inputs
and outputs are different.
There is an opportunity for more refactoring and streamlining if we look
at the flow of arguments through the whole call-chain (as per my other
email) but that's a bit beyond the scope of this initial conversion I think.
Cheers,
David
-----
I do not see any issues, so it seem reasonable, thanks.
Thanks, Robbin
Thanks,
Yasumasa
On 2020/06/30 19:23, Robbin Ehn wrote:
Hi Yasumasa,
Thanks for your effort doing this.
#1
GetFrameLocation
GetStackTrace
GetCurrentLocation (need to add BCI)
Should use exactly the same code since a stack trace with max_count = 1
and start_depth = depth/0 is the frame location and jvmtiFrameInfo
contain the correct information (+ add BCI)? Thus GetFrameLocation also
would use handshakes and no special handshake path for
GetCurrentLocation.
So we would have _one_ function to get method and BCI/lineno for
depth and max count. Which can easily handle all three cases? (maybe
more
cases also)
Is there nay reason for having a separate path for each of these ???
#2
In this method:
JvmtiEnvThreadState::reset_current_location(jvmtiEvent event_type,
bool enabled)
if (event_type == JVMTI_EVENT_SINGLE_STEP &&
_thread->has_last_Java_frame()) {
We are checking if a running thread have a last Java frame, which
means it could have one now, e.g. it could be in another handshake or
not woken up from a safepoint yet. So there is no use in checking that.
(old code)
313 if (SafepointSynchronize::is_at_safepoint() ||
314 ((Thread::current() == _thread) && (_thread ==
_thread->active_handshaker()))) {
#3
You are using a debug only method here "active_handshaker()".
#4
This AND is never true:
((Thread::current() == _thread) && (_thread ==
_thread->active_handshaker())))
You can't be active handshaker for yourself.
Thanks, Robbin
On 2020-06-24 08:50, 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/
This change replace following VM operations to direct handshake.
- VM_GetFrameCount (GetFrameCount())
- VM_GetFrameLocation (GetFrameLocation())
- VM_GetThreadListStackTraces (GetThreadListStackTrace())
- VM_GetCurrentLocation
GetThreadListStackTrace() uses direct handshake if thread count ==
1. In other case (thread count > 1), it would be performed as VM
operation (VM_GetThreadListStackTraces).
Caller of VM_GetCurrentLocation
(JvmtiEnvThreadState::reset_current_location()) might be called at
safepoint. So I added safepoint check in its caller.
This change has been tested in serviceability/jvmti
serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi
vmTestbase/ns
k/jdwp.
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
