Re: RFR(M): 8244383: jhsdb/HeapDumpTestWithActiveProcess.java fails with "AssertionFailure: illegal bci"
Hi Chris,
The fix, in general, looks good to me.
Some small comments for
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/amd64/AMD64CurrentFrameGuess.java:
1) Imports at lines 30 and 35 are not used and could be removed.
30 import sun.jvm.hotspot.interpreter.*;
35 import sun.jvm.hotspot.utilities.*;
2) Local variable "end" defined at line 189 is not used.
189 Address end = sp.addOffsetTo(regionInBytesToSearch);
No new webrev is required.
Thanks,
Daniil
On 6/25/20, 1:55 PM, "serviceability-dev on behalf of Chris Plummer"
wrote:
Ping #2. I still need one more reviewer (Thanks for the review, Dan). I
updated the webrev based on Dan's comments:
http://cr.openjdk.java.net/~cjplummer/8244383/webrev.01/
I can still make the simplification mentioned below if necessary.
thanks,
Chris
On 6/23/20 11:29 AM, Chris Plummer wrote:
> Ping!
>
> If this fix is too complicated, there is a simplification I can make,
> but at the cost of abandoning some attempts to determine the current
> frame when this error condition pops up. At the start of
> validateInterpreterFrame() it attempts to verify that the frame is
> valid by verifying that frame->method and frame->bcp are valid. This
> part is pretty simple. The complicated part is everything that follows
> if the verification fails. It attempts to error correct the situation
> by looking at various register contents and stack contents. I could
> just abandon this complicated code and return false if frame->method
> and frame->bcp don't check out. Upon return, the caller's code would
> be simplified to:
>
> if (validateInterpreterFrame(sp, fp, pc)) {
> return true; // We're done. setValues() has been called
> for valid interpreter frame.
> } else {
> return checkLastJavaSP();
> }
>
> So there's still a chance we can determine a valid current frame if
> "last java frame" has been setup. However, if not setup we would not
> be able to. This is where the complicated code in
> validateInterpreterFrame() is useful because it can usually determine
> the current frame, even if "last java frame" is not setup, but it's
> rare enough that we run into this situation that I think failing to
> get the current frame is ok.
>
> So if I can get a couple promises for reviews if I make this change,
> I'll go ahead and do it and send out a new RFR.
>
> thanks,
>
> Chris
>
> On 6/18/20 5:54 PM, Chris Plummer wrote:
>> [I've added runtime-dev to this SA review since understanding
>> interpreter invokes (code generated by
>> TemplateInterpreterGenerator::generate_normal_entry()) and stack
>> walking is probably more important than understanding SA.]
>>
>> Hello,
>>
>> Please help review the following:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8244383
>> http://cr.openjdk.java.net/~cjplummer/8244383/webrev.00/index.html
>>
>> The crux of the bug is when doing stack walking the topmost frame is
>> in an inconsistent state because we are in the middle of pushing a
>> new interpreter frame. Basically we are executing code generated by
>> TemplateInterpreterGenerator::generate_normal_entry(). Since the PC
>> register is in this code, SA assumes the topmost frame is an
>> interpreter frame.
>>
>> The first issue with this interpreter frame assumption is if we
>> haven't actually pushed the frame yet, then the current frame is the
>> caller's frame, and could be compiled. But since SA thinks it's
>> interpreted, later on it tries to convert the frame->bcp to a BCI,
>> but frame->bcp is only valid for interpreter frames. Thus the
>> "illegal BCI" failures. If the previous frame happened to be
>> interpreted, then the existing SA code works fine.
>>
>> The other state of frame pushing that was problematic was when the
>> new frame had been pushed, but frame->method and frame->bcp were not
>> setup yet. This also would lead to "illegal BCI" later on because
>> garbage would be stored in these locations.
>>
>> Fixing the above problems requires trying to determine the state of
>> the frame push through a series of checks, and then adapting what is
>> considered to be the current frame based on the outcome of the
>> checks. The first things checked is that frame->method is valid (we
>> can successfully instantiate a wrapper for the Method* without
>> failure) and that frame->bcp is within the method. If both these pass
>> then we can use the frame as-is.
>>
>> If the above checks fail, then we try to determine whether the issue
>> is that the frame is not yet pushed and the current frame is ac
Re: RFR(S): 8247533: SA stack walking sometimes fails with sun.jvm.hotspot.debugger.DebuggerException: get_thread_regs failed for a lwp
Hi Chris, The fix looks good to me. Best regards, Daniil On 6/25/20 13:29, Chris Plummer wrote: > Ping. I still need one more review for this. There was one updated > webev. I list it below so you don't need to dig it up in the long > email thread: >> I've updated with webrev based on the new finding that a JavaThread >> cannot be on the ThreadList after its OS thread has been destroyed >> since the JavaThread removes itself from the ThreadList, and >> therefore must be running on its OS thread. The logic of the fix is >> unchanged from the first webrev, but I updated the comments to better >> reflect what is going on. I also updated the CR: >> >> https://bugs.openjdk.java.net/browse/JDK-8247533 >> http://cr.openjdk.java.net/~cjplummer/8247533/webrev.01/index.html > > thanks, > > Chris > > On 6/17/20 1:34 PM, Chris Plummer wrote: >> Hello, >> >> Please help review the following: >> >> https://bugs.openjdk.java.net/browse/JDK-8247533 >> http://cr.openjdk.java.net/~cjplummer/8247533/webrev.00/index.html >> >> The CR contains all the needed details. Here's a summary of changes >> in each file: >> >> src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.cpp >> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m >> src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp >> -Instead of throwing an exception when the OS ThreadID is invalid, >> print a warning. >> >> src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c >> -Improve a print_debug message >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdThread.java >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/LinuxThread.java >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/windbg/amd64/WindbgAMD64Thread.java >> >> -Deal with the array of registers read in being null due to the OS >> ThreadID not being valid. >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/LinuxDebuggerLocal.java >> >> -Fix issue with "sun.jvm.hotspot.debugger.DebuggerException" >> appearing twice when printing the exception. >> >> thanks, >> >> Chris > >
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Hi,
I uploaded new webrev. Could review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.02/
src/hotspot/share/prims/jvmtiEnvBase.cpp
820 assert(SafepointSynchronize::is_at_safepoint() ||
821 java_thread->is_thread_fully_suspended(false, &debug_bits) ||
822 current_thread == java_thread->active_handshaker(),
823 "at safepoint / handshake or target thread is suspended");
I don't think the suspension check is necessary, as even if the target is
suspended we must still be at a safepoint or in a handshake with it. Makes me
wonder if we used to allow a racy stacktrace operation on a suspended thread,
assuming it would remain suspended?
This function (JvmtiEnvBase::get_stack_trace()) can be called to get own stack
trace. For example, we can call GetStackTrace() for current thread at JVMTI
event.
So I changed assert as below:
```
820 assert(current_thread == java_thread ||
821 SafepointSynchronize::is_at_safepoint() ||
822 current_thread == java_thread->active_handshaker(),
823 "call by myself / at safepoint / at handshake");
```
Thanks,
Yasumasa
On 2020/07/01 8:48, David Holmes wrote:
Hi Yasumasa,
On 1/07/2020 9:05 am, Yasumasa Suenaga wrote:
Hi David,
1271 ResourceMark rm;
IIUC at this point the _calling_thread is the current thread, so we can use:
ResourceMark rm(_calling_thread);
If so, we can call make_local() in L1272 without JavaThread (or we can pass
current thread to make_local()). Is it right?
```
1271 ResourceMark rm;
1272 _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread,
thread_oop),
1273 jt, thread_oop);
```
Sorry I got confused, _calling_thread may not be the current thread as we could
be executing the handshake in the target thread itself. So the ResourceMark is
correct as-is (implicitly for current thread).
The argument to fill_frames will be used in the jvmtiStackInfo and passed back
to the _calling_thread, so it must be created via make_local(_calling_thread,
...) as you presently have.
Thanks,
David
Thanks,
Yasumasa
On 2020/07/01 7:05, David Holmes wrote:
On 1/07/2020 12:17 am, Yasumasa Suenaga wrote:
Hi David,
Thank you for reviewing! I will update new webrev tomorrow.
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as it may not
be on the stack. I think MultipleStackTracesCollector should not extend any
allocation class, and should always be embedded directly in another class.
I'm not sure what does mean "embedded".
Is it ok as below?
```
class MultipleStackTracesCollector {
:
}
class GetAllStackTraces : public VM_Operation {
private:
MultipleStackTracesCollector _collector;
}
```
Yes that I what I meant.
Thanks,
David
-
Thanks,
Yasumasa
On 2020/06/30 22:22, David Holmes wrote:
Hi Yasumasa,
On 30/06/2020 10:05 am, Yasumasa Suenaga wrote:
Hi David, Serguei,
I updated webrev for 8242428. Could you review again?
This change migrate to use direct handshake for GetStackTrace() and
GetThreadListStackTraces() (when thread_count == 1).
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
This looks really good now! I only have a few nits below. There is one thing I
don't like about it but it requires a change to the main Handshake logic to
address - in JvmtiEnv::GetThreadListStackTraces you have to create a
ThreadsListHandle to convert the jthread to a JavaThread, but then the
Handshake::execute_direct creates another ThreadsListHandle internally. That's
a waste. I will discuss with Robbin and file a RFE to have an overload of
execute_direct that takes an existing TLH. Actually it's worse than that
because we have another TLH in use at the entry point for the JVMTI functions,
so I think there may be some scope for simplifying the use of TLH instances -
future RFE.
---
src/hotspot/share/prims/jvmtiEnvBase.hpp
451 GetStackTraceClosure(JvmtiEnv *env, jint start_depth, jint max_count,
452 jvmtiFrameInfo* frame_buffer, jint* count_ptr)
453 : HandshakeClosure("GetStackTrace"),
454 _env(env), _start_depth(start_depth), _max_count(max_count),
455 _frame_buffer(frame_buffer), _count_ptr(count_ptr),
456 _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {
Nit: can you do one initializer per line please.
This looks wrong:
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as it may
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
On 2020/07/01 8:54, David Holmes wrote: On 1/07/2020 9:51 am, Yasumasa Suenaga wrote: Hi Serguei, On 2020/07/01 8:24, serguei.spit...@oracle.com wrote: Hi Yasumasa, Thank you for separating your initial webrev. I'll do a full review after you address comments from David and Robbin as I'm stepping on the same ground. Just a quick comment now. http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html I've already asked in prev. round to make this renaming: target_javathread => java_thread The identifier java_thread is normally used in the jvmtiEnv.cpp functions. The target_javathread sounds very unusual. Sorry, I've fixed it. I will update webrev. I do not like much the introduction ofthe GetSingleStackTraceClosure. It feels like it can be done in a more elegant way. From the other hand, it is not that bad. :-) I thought to use handshake on VMThread (!= direct handshake) for GetThreadListStackTraces() to be simplify implementation. However it would be queued as VM op (of course STW would not happen), so I introduced GetSingleStackTraceClosure. Getting multiple stacktraces must be a stop-the-world operation, so that the traces are consistent. Yes, this is the case of thread_count == 1. Yasumasa David Thanks, Yasumasa Thanks, Serguei On 6/29/20 17:05, Yasumasa Suenaga wrote: Hi David, Serguei, I updated webrev for 8242428. Could you review again? This change migrate to use direct handshake for GetStackTrace() and GetThreadListStackTraces() (when thread_count == 1). http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/ VM_GetThreadListStackTrace (for GetThreadListStackTraces) and VM_GetAllStackTraces (for GetAllStackTraces) have inherited VM_GetMultipleStackTraces VM operation which provides the feature to generate jvmtiStackInfo. I modified VM_GetMultipleStackTraces to a normal C++ class to share with HandshakeClosure for GetThreadListStackTraces (GetSingleStackTraceClosure). Also I added new testcases which test GetThreadListStackTraces() with thread_count == 1 and with all threads. This change has been tested in serviceability/jvmti serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/nsk/jdwp. Thanks, Yasumasa On 2020/06/24 15: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
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
On 1/07/2020 9:51 am, Yasumasa Suenaga wrote: Hi Serguei, On 2020/07/01 8:24, serguei.spit...@oracle.com wrote: Hi Yasumasa, Thank you for separating your initial webrev. I'll do a full review after you address comments from David and Robbin as I'm stepping on the same ground. Just a quick comment now. http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html I've already asked in prev. round to make this renaming: target_javathread => java_thread The identifier java_thread is normally used in the jvmtiEnv.cpp functions. The target_javathread sounds very unusual. Sorry, I've fixed it. I will update webrev. I do not like much the introduction ofthe GetSingleStackTraceClosure. It feels like it can be done in a more elegant way. From the other hand, it is not that bad. :-) I thought to use handshake on VMThread (!= direct handshake) for GetThreadListStackTraces() to be simplify implementation. However it would be queued as VM op (of course STW would not happen), so I introduced GetSingleStackTraceClosure. Getting multiple stacktraces must be a stop-the-world operation, so that the traces are consistent. David Thanks, Yasumasa Thanks, Serguei On 6/29/20 17:05, Yasumasa Suenaga wrote: Hi David, Serguei, I updated webrev for 8242428. Could you review again? This change migrate to use direct handshake for GetStackTrace() and GetThreadListStackTraces() (when thread_count == 1). http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/ VM_GetThreadListStackTrace (for GetThreadListStackTraces) and VM_GetAllStackTraces (for GetAllStackTraces) have inherited VM_GetMultipleStackTraces VM operation which provides the feature to generate jvmtiStackInfo. I modified VM_GetMultipleStackTraces to a normal C++ class to share with HandshakeClosure for GetThreadListStackTraces (GetSingleStackTraceClosure). Also I added new testcases which test GetThreadListStackTraces() with thread_count == 1 and with all threads. This change has been tested in serviceability/jvmti serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/nsk/jdwp. Thanks, Yasumasa On 2020/06/24 15: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
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Hi Serguei, On 2020/07/01 8:24, serguei.spit...@oracle.com wrote: Hi Yasumasa, Thank you for separating your initial webrev. I'll do a full review after you address comments from David and Robbin as I'm stepping on the same ground. Just a quick comment now. http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html I've already asked in prev. round to make this renaming: target_javathread => java_thread The identifier java_thread is normally used in the jvmtiEnv.cpp functions. The target_javathread sounds very unusual. Sorry, I've fixed it. I will update webrev. I do not like much the introduction ofthe GetSingleStackTraceClosure. It feels like it can be done in a more elegant way. From the other hand, it is not that bad. :-) I thought to use handshake on VMThread (!= direct handshake) for GetThreadListStackTraces() to be simplify implementation. However it would be queued as VM op (of course STW would not happen), so I introduced GetSingleStackTraceClosure. Thanks, Yasumasa Thanks, Serguei On 6/29/20 17:05, Yasumasa Suenaga wrote: Hi David, Serguei, I updated webrev for 8242428. Could you review again? This change migrate to use direct handshake for GetStackTrace() and GetThreadListStackTraces() (when thread_count == 1). http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/ VM_GetThreadListStackTrace (for GetThreadListStackTraces) and VM_GetAllStackTraces (for GetAllStackTraces) have inherited VM_GetMultipleStackTraces VM operation which provides the feature to generate jvmtiStackInfo. I modified VM_GetMultipleStackTraces to a normal C++ class to share with HandshakeClosure for GetThreadListStackTraces (GetSingleStackTraceClosure). Also I added new testcases which test GetThreadListStackTraces() with thread_count == 1 and with all threads. This change has been tested in serviceability/jvmti serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/nsk/jdwp. Thanks, Yasumasa On 2020/06/24 15: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
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Hi Yasumasa,
On 1/07/2020 9:05 am, Yasumasa Suenaga wrote:
Hi David,
1271 ResourceMark rm;
IIUC at this point the _calling_thread is the current thread, so we
can use:
ResourceMark rm(_calling_thread);
If so, we can call make_local() in L1272 without JavaThread (or we can
pass current thread to make_local()). Is it right?
```
1271 ResourceMark rm;
1272
_collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread,
thread_oop),
1273 jt, thread_oop);
```
Sorry I got confused, _calling_thread may not be the current thread as
we could be executing the handshake in the target thread itself. So the
ResourceMark is correct as-is (implicitly for current thread).
The argument to fill_frames will be used in the jvmtiStackInfo and
passed back to the _calling_thread, so it must be created via
make_local(_calling_thread, ...) as you presently have.
Thanks,
David
Thanks,
Yasumasa
On 2020/07/01 7:05, David Holmes wrote:
On 1/07/2020 12:17 am, Yasumasa Suenaga wrote:
Hi David,
Thank you for reviewing! I will update new webrev tomorrow.
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as
it may not be on the stack. I think MultipleStackTracesCollector
should not extend any allocation class, and should always be
embedded directly in another class.
I'm not sure what does mean "embedded".
Is it ok as below?
```
class MultipleStackTracesCollector {
:
}
class GetAllStackTraces : public VM_Operation {
private:
MultipleStackTracesCollector _collector;
}
```
Yes that I what I meant.
Thanks,
David
-
Thanks,
Yasumasa
On 2020/06/30 22:22, David Holmes wrote:
Hi Yasumasa,
On 30/06/2020 10:05 am, Yasumasa Suenaga wrote:
Hi David, Serguei,
I updated webrev for 8242428. Could you review again?
This change migrate to use direct handshake for GetStackTrace() and
GetThreadListStackTraces() (when thread_count == 1).
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
This looks really good now! I only have a few nits below. There is
one thing I don't like about it but it requires a change to the main
Handshake logic to address - in JvmtiEnv::GetThreadListStackTraces
you have to create a ThreadsListHandle to convert the jthread to a
JavaThread, but then the Handshake::execute_direct creates another
ThreadsListHandle internally. That's a waste. I will discuss with
Robbin and file a RFE to have an overload of execute_direct that
takes an existing TLH. Actually it's worse than that because we have
another TLH in use at the entry point for the JVMTI functions, so I
think there may be some scope for simplifying the use of TLH
instances - future RFE.
---
src/hotspot/share/prims/jvmtiEnvBase.hpp
451 GetStackTraceClosure(JvmtiEnv *env, jint start_depth, jint
max_count,
452 jvmtiFrameInfo* frame_buffer, jint*
count_ptr)
453 : HandshakeClosure("GetStackTrace"),
454 _env(env), _start_depth(start_depth),
_max_count(max_count),
455 _frame_buffer(frame_buffer), _count_ptr(count_ptr),
456 _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {
Nit: can you do one initializer per line please.
This looks wrong:
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as
it may not be on the stack. I think MultipleStackTracesCollector
should not extend any allocation class, and should always be
embedded directly in another class.
481 MultipleStackTracesCollector(JvmtiEnv *env, jint
max_frame_count) {
482 _env = env;
483 _max_frame_count = max_frame_count;
484 _frame_count_total = 0;
485 _head = NULL;
486 _stack_info = NULL;
487 _result = JVMTI_ERROR_NONE;
488 }
As you are touching this can you change it to use an initializer
list as you did for the HandshakeClosure, and please keep one item
per line.
---
src/hotspot/share/prims/jvmtiEnvBase.cpp
820 assert(SafepointSynchronize::is_at_safepoint() ||
821 java_thread->is_thread_fully_suspended(false,
&debug_bits) ||
822 current_thread == java_thread->active_handshaker(),
823 "at safepoint / handshake or target thread is
suspended");
I don't think the suspension check is necessary, as even if the
target is suspended we must still be at a safepoint or in a
handshake with it. Makes me wonder if we used to allow a racy
stacktrace operation on a suspended thread, assuming it would remain
suspende
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Hi Yasumasa, Thank you for separating your initial webrev. I'll do a full review after you address comments from David and Robbin as I'm stepping on the same ground. Just a quick comment now. http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html I've already asked in prev. round to make this renaming: target_javathread => java_thread The identifier java_thread is normally used in the jvmtiEnv.cpp functions. The target_javathread sounds very unusual. I do not like much the introduction of the GetSingleStackTraceClosure. It feels like it can be done in a more elegant way. From the other hand, it is not that bad. :-) Thanks, Serguei On 6/29/20 17:05, Yasumasa Suenaga wrote: Hi David, Serguei, I updated webrev for 8242428. Could you review again? This change migrate to use direct handshake for GetStackTrace() and GetThreadListStackTraces() (when thread_count == 1). http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/ VM_GetThreadListStackTrace (for GetThreadListStackTraces) and VM_GetAllStackTraces (for GetAllStackTraces) have inherited VM_GetMultipleStackTraces VM operation which provides the feature to generate jvmtiStackInfo. I modified VM_GetMultipleStackTraces to a normal C++ class to share with HandshakeClosure for GetThreadListStackTraces (GetSingleStackTraceClosure). Also I added new testcases which test GetThreadListStackTraces() with thread_count == 1 and with all threads. This change has been tested in serviceability/jvmti serviceability/jdwp vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/nsk/jdwp. Thanks, Yasumasa On 2020/06/24 15: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
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Hi David,
1271 ResourceMark rm;
IIUC at this point the _calling_thread is the current thread, so we can use:
ResourceMark rm(_calling_thread);
If so, we can call make_local() in L1272 without JavaThread (or we can pass
current thread to make_local()). Is it right?
```
1271 ResourceMark rm;
1272
_collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread,
thread_oop),
1273jt, thread_oop);
```
Thanks,
Yasumasa
On 2020/07/01 7:05, David Holmes wrote:
On 1/07/2020 12:17 am, Yasumasa Suenaga wrote:
Hi David,
Thank you for reviewing! I will update new webrev tomorrow.
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as it may not
be on the stack. I think MultipleStackTracesCollector should not extend any
allocation class, and should always be embedded directly in another class.
I'm not sure what does mean "embedded".
Is it ok as below?
```
class MultipleStackTracesCollector {
:
}
class GetAllStackTraces : public VM_Operation {
private:
MultipleStackTracesCollector _collector;
}
```
Yes that I what I meant.
Thanks,
David
-
Thanks,
Yasumasa
On 2020/06/30 22:22, David Holmes wrote:
Hi Yasumasa,
On 30/06/2020 10:05 am, Yasumasa Suenaga wrote:
Hi David, Serguei,
I updated webrev for 8242428. Could you review again?
This change migrate to use direct handshake for GetStackTrace() and
GetThreadListStackTraces() (when thread_count == 1).
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
This looks really good now! I only have a few nits below. There is one thing I
don't like about it but it requires a change to the main Handshake logic to
address - in JvmtiEnv::GetThreadListStackTraces you have to create a
ThreadsListHandle to convert the jthread to a JavaThread, but then the
Handshake::execute_direct creates another ThreadsListHandle internally. That's
a waste. I will discuss with Robbin and file a RFE to have an overload of
execute_direct that takes an existing TLH. Actually it's worse than that
because we have another TLH in use at the entry point for the JVMTI functions,
so I think there may be some scope for simplifying the use of TLH instances -
future RFE.
---
src/hotspot/share/prims/jvmtiEnvBase.hpp
451 GetStackTraceClosure(JvmtiEnv *env, jint start_depth, jint max_count,
452 jvmtiFrameInfo* frame_buffer, jint* count_ptr)
453 : HandshakeClosure("GetStackTrace"),
454 _env(env), _start_depth(start_depth), _max_count(max_count),
455 _frame_buffer(frame_buffer), _count_ptr(count_ptr),
456 _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {
Nit: can you do one initializer per line please.
This looks wrong:
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as it may not
be on the stack. I think MultipleStackTracesCollector should not extend any
allocation class, and should always be embedded directly in another class.
481 MultipleStackTracesCollector(JvmtiEnv *env, jint max_frame_count) {
482 _env = env;
483 _max_frame_count = max_frame_count;
484 _frame_count_total = 0;
485 _head = NULL;
486 _stack_info = NULL;
487 _result = JVMTI_ERROR_NONE;
488 }
As you are touching this can you change it to use an initializer list as you
did for the HandshakeClosure, and please keep one item per line.
---
src/hotspot/share/prims/jvmtiEnvBase.cpp
820 assert(SafepointSynchronize::is_at_safepoint() ||
821 java_thread->is_thread_fully_suspended(false, &debug_bits) ||
822 current_thread == java_thread->active_handshaker(),
823 "at safepoint / handshake or target thread is suspended");
I don't think the suspension check is necessary, as even if the target is
suspended we must still be at a safepoint or in a handshake with it. Makes me
wonder if we used to allow a racy stacktrace operation on a suspended thread,
assuming it would remain suspended?
1268 oop thread_oop = jt->threadObj();
1269
1270 if (!jt->is_exiting() && (jt->threadObj() != NULL)) {
You can use thread_oop in line 1270.
1272 _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread,
thread_oop),
1273 jt, thread_oop);
It is frustrating that this entire call chain started with a jthread reference,
which we converted to a JavaThread, only to eventually need to convert it back
to a jthread! I think there is some scope for sim
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
On 1/07/2020 12:17 am, Yasumasa Suenaga wrote:
Hi David,
Thank you for reviewing! I will update new webrev tomorrow.
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as it
may not be on the stack. I think MultipleStackTracesCollector should
not extend any allocation class, and should always be embedded
directly in another class.
I'm not sure what does mean "embedded".
Is it ok as below?
```
class MultipleStackTracesCollector {
:
}
class GetAllStackTraces : public VM_Operation {
private:
MultipleStackTracesCollector _collector;
}
```
Yes that I what I meant.
Thanks,
David
-
Thanks,
Yasumasa
On 2020/06/30 22:22, David Holmes wrote:
Hi Yasumasa,
On 30/06/2020 10:05 am, Yasumasa Suenaga wrote:
Hi David, Serguei,
I updated webrev for 8242428. Could you review again?
This change migrate to use direct handshake for GetStackTrace() and
GetThreadListStackTraces() (when thread_count == 1).
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
This looks really good now! I only have a few nits below. There is one
thing I don't like about it but it requires a change to the main
Handshake logic to address - in JvmtiEnv::GetThreadListStackTraces you
have to create a ThreadsListHandle to convert the jthread to a
JavaThread, but then the Handshake::execute_direct creates another
ThreadsListHandle internally. That's a waste. I will discuss with
Robbin and file a RFE to have an overload of execute_direct that takes
an existing TLH. Actually it's worse than that because we have another
TLH in use at the entry point for the JVMTI functions, so I think
there may be some scope for simplifying the use of TLH instances -
future RFE.
---
src/hotspot/share/prims/jvmtiEnvBase.hpp
451 GetStackTraceClosure(JvmtiEnv *env, jint start_depth, jint
max_count,
452 jvmtiFrameInfo* frame_buffer, jint*
count_ptr)
453 : HandshakeClosure("GetStackTrace"),
454 _env(env), _start_depth(start_depth), _max_count(max_count),
455 _frame_buffer(frame_buffer), _count_ptr(count_ptr),
456 _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {
Nit: can you do one initializer per line please.
This looks wrong:
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as it
may not be on the stack. I think MultipleStackTracesCollector should
not extend any allocation class, and should always be embedded
directly in another class.
481 MultipleStackTracesCollector(JvmtiEnv *env, jint max_frame_count) {
482 _env = env;
483 _max_frame_count = max_frame_count;
484 _frame_count_total = 0;
485 _head = NULL;
486 _stack_info = NULL;
487 _result = JVMTI_ERROR_NONE;
488 }
As you are touching this can you change it to use an initializer list
as you did for the HandshakeClosure, and please keep one item per line.
---
src/hotspot/share/prims/jvmtiEnvBase.cpp
820 assert(SafepointSynchronize::is_at_safepoint() ||
821 java_thread->is_thread_fully_suspended(false,
&debug_bits) ||
822 current_thread == java_thread->active_handshaker(),
823 "at safepoint / handshake or target thread is suspended");
I don't think the suspension check is necessary, as even if the target
is suspended we must still be at a safepoint or in a handshake with
it. Makes me wonder if we used to allow a racy stacktrace operation on
a suspended thread, assuming it would remain suspended?
1268 oop thread_oop = jt->threadObj();
1269
1270 if (!jt->is_exiting() && (jt->threadObj() != NULL)) {
You can use thread_oop in line 1270.
1272
_collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread, thread_oop),
1273 jt, thread_oop);
It is frustrating that this entire call chain started with a jthread
reference, which we converted to a JavaThread, only to eventually need
to convert it back to a jthread! I think there is some scope for
simplification here but not as part of this change.
1271 ResourceMark rm;
IIUC at this point the _calling_thread is the current thread, so we
can use:
ResourceMark rm(_calling_thread);
---
Please add @bug lines to the tests.
I'm still pondering the test logic but wanted to send this now.
Thanks,
David
-
VM_GetThreadListStackTrace (for GetThreadListStackTraces) and
VM_GetAllStackTraces (for GetAllStackTraces) have inherited
VM_GetMultipleStackTraces VM operation which provides the
Re: RFR(S): 8247533: SA stack walking sometimes fails with sun.jvm.hotspot.debugger.DebuggerException: get_thread_regs failed for a lwp
Thanks! On 6/30/20 11:57 AM, serguei.spit...@oracle.com wrote: Hi Chris, I do not see any problems with this change. Thanks, Serguei On 6/25/20 13:29, Chris Plummer wrote: Ping. I still need one more review for this. There was one updated webev. I list it below so you don't need to dig it up in the long email thread: I've updated with webrev based on the new finding that a JavaThread cannot be on the ThreadList after its OS thread has been destroyed since the JavaThread removes itself from the ThreadList, and therefore must be running on its OS thread. The logic of the fix is unchanged from the first webrev, but I updated the comments to better reflect what is going on. I also updated the CR: https://bugs.openjdk.java.net/browse/JDK-8247533 http://cr.openjdk.java.net/~cjplummer/8247533/webrev.01/index.html thanks, Chris On 6/17/20 1:34 PM, Chris Plummer wrote: Hello, Please help review the following: https://bugs.openjdk.java.net/browse/JDK-8247533 http://cr.openjdk.java.net/~cjplummer/8247533/webrev.00/index.html The CR contains all the needed details. Here's a summary of changes in each file: src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.cpp src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp -Instead of throwing an exception when the OS ThreadID is invalid, print a warning. src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c -Improve a print_debug message src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdThread.java src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/LinuxThread.java src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/windbg/amd64/WindbgAMD64Thread.java -Deal with the array of registers read in being null due to the OS ThreadID not being valid. src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/LinuxDebuggerLocal.java -Fix issue with "sun.jvm.hotspot.debugger.DebuggerException" appearing twice when printing the exception. thanks, Chris
Re: RFR(S): 8247533: SA stack walking sometimes fails with sun.jvm.hotspot.debugger.DebuggerException: get_thread_regs failed for a lwp
Hi Chris, I do not see any problems with this change. Thanks, Serguei On 6/25/20 13:29, Chris Plummer wrote: Ping. I still need one more review for this. There was one updated webev. I list it below so you don't need to dig it up in the long email thread: I've updated with webrev based on the new finding that a JavaThread cannot be on the ThreadList after its OS thread has been destroyed since the JavaThread removes itself from the ThreadList, and therefore must be running on its OS thread. The logic of the fix is unchanged from the first webrev, but I updated the comments to better reflect what is going on. I also updated the CR: https://bugs.openjdk.java.net/browse/JDK-8247533 http://cr.openjdk.java.net/~cjplummer/8247533/webrev.01/index.html thanks, Chris On 6/17/20 1:34 PM, Chris Plummer wrote: Hello, Please help review the following: https://bugs.openjdk.java.net/browse/JDK-8247533 http://cr.openjdk.java.net/~cjplummer/8247533/webrev.00/index.html The CR contains all the needed details. Here's a summary of changes in each file: src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.cpp src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp -Instead of throwing an exception when the OS ThreadID is invalid, print a warning. src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c -Improve a print_debug message src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdThread.java src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/LinuxThread.java src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/windbg/amd64/WindbgAMD64Thread.java -Deal with the array of registers read in being null due to the OS ThreadID not being valid. src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/LinuxDebuggerLocal.java -Fix issue with "sun.jvm.hotspot.debugger.DebuggerException" appearing twice when printing the exception. thanks, Chris
RE: RFR: 8227337: javax/management/remote/mandatory/connection/ReconnectTest.java NoSuchObjectException no such object in table
The JBS issue is non-public, but this looks fine. I assume you set test.timeout.factor to something larger than 1.0 when you run MultiThreadDeadLockTest. Thanks, Paul On 6/29/20, 12:43 PM, "serviceability-dev on behalf of Daniil Titov" wrote: Please review the change that fixes an intermittent tests failure. The tests javax/management/remote/mandatory/connection/ReconnectTest.java and javax/management/remote/mandatory/connection/MultiThreadDeadLockTest.java use specific settings for server timeout that in some cases (e.g. when the test is run with -Xcomp) result in JMX server connection timeout thread unexports the remote object while the client connection is still in the progress. Below is an example of a such stacktrace: java.rmi.NoSuchObjectException: no such object in table at java.rmi/sun.rmi.transport.StreamRemoteCall.exceptionReceivedFromServer(StreamRemoteCall.java:303) at java.rmi/sun.rmi.transport.StreamRemoteCall.executeCall(StreamRemoteCall.java:279) at java.rmi/sun.rmi.server.UnicastRef.invoke(UnicastRef.java:164) at jdk.remoteref/jdk.jmx.remote.internal.rmi.PRef.invoke(Unknown Source) at java.management.rmi/javax.management.remote.rmi.RMIConnectionImpl_Stub.getConnectionId(RMIConnectionImpl_Stub.java:318) at java.management.rmi/javax.management.remote.rmi.RMIConnector.getConnectionId(RMIConnector.java:385) at java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:347) at java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:270) at MultiThreadDeadLockTest.main(MultiThreadDeadLockTest.java:87) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:564) at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) at java.base/java.lang.Thread.run(Thread.java:832) The fix adjusts the server timeout the tests use for "test.timeout.factor" system property. Testing: Mach5 tests are in the progress. [1] https://cr.openjdk.java.net/~dtitov/8227337/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8227337 Thanks, Daniil
RE: RFR: 8205467: javax/management/remote/mandatory/connection/MultiThreadDeadLockTest.java possible deadlock
Lgtm. Thanks, Paul On 6/29/20, 12:58 PM, "serviceability-dev on behalf of Daniil Titov" wrote: Please review a tiny change that adjusts the wait timeout the test uses for "test.timeout.factor" system property. Please note that a trivial merge with fix [4] that is currently on review [3] will be required. Since issues [2] and [4] describe different problems I decided to not combine these both changes in the single fix. Testing: Mach5 tests tier1-tier3 successfully passed. [1] Web rev: https://cr.openjdk.java.net/~dtitov/8205467/webrev.01/ [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8205467 [3] https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/032098.html [4] https://bugs.openjdk.java.net/browse/JDK-8227337 Thank you, Daniil
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Hi David,
Thank you for reviewing! I will update new webrev tomorrow.
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as it may not
be on the stack. I think MultipleStackTracesCollector should not extend any
allocation class, and should always be embedded directly in another class.
I'm not sure what does mean "embedded".
Is it ok as below?
```
class MultipleStackTracesCollector {
:
}
class GetAllStackTraces : public VM_Operation {
private:
MultipleStackTracesCollector _collector;
}
```
Thanks,
Yasumasa
On 2020/06/30 22:22, David Holmes wrote:
Hi Yasumasa,
On 30/06/2020 10:05 am, Yasumasa Suenaga wrote:
Hi David, Serguei,
I updated webrev for 8242428. Could you review again?
This change migrate to use direct handshake for GetStackTrace() and
GetThreadListStackTraces() (when thread_count == 1).
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
This looks really good now! I only have a few nits below. There is one thing I
don't like about it but it requires a change to the main Handshake logic to
address - in JvmtiEnv::GetThreadListStackTraces you have to create a
ThreadsListHandle to convert the jthread to a JavaThread, but then the
Handshake::execute_direct creates another ThreadsListHandle internally. That's
a waste. I will discuss with Robbin and file a RFE to have an overload of
execute_direct that takes an existing TLH. Actually it's worse than that
because we have another TLH in use at the entry point for the JVMTI functions,
so I think there may be some scope for simplifying the use of TLH instances -
future RFE.
---
src/hotspot/share/prims/jvmtiEnvBase.hpp
451 GetStackTraceClosure(JvmtiEnv *env, jint start_depth, jint max_count,
452 jvmtiFrameInfo* frame_buffer, jint* count_ptr)
453 : HandshakeClosure("GetStackTrace"),
454 _env(env), _start_depth(start_depth), _max_count(max_count),
455 _frame_buffer(frame_buffer), _count_ptr(count_ptr),
456 _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {
Nit: can you do one initializer per line please.
This looks wrong:
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as it may not
be on the stack. I think MultipleStackTracesCollector should not extend any
allocation class, and should always be embedded directly in another class.
481 MultipleStackTracesCollector(JvmtiEnv *env, jint max_frame_count) {
482 _env = env;
483 _max_frame_count = max_frame_count;
484 _frame_count_total = 0;
485 _head = NULL;
486 _stack_info = NULL;
487 _result = JVMTI_ERROR_NONE;
488 }
As you are touching this can you change it to use an initializer list as you
did for the HandshakeClosure, and please keep one item per line.
---
src/hotspot/share/prims/jvmtiEnvBase.cpp
820 assert(SafepointSynchronize::is_at_safepoint() ||
821 java_thread->is_thread_fully_suspended(false, &debug_bits) ||
822 current_thread == java_thread->active_handshaker(),
823 "at safepoint / handshake or target thread is suspended");
I don't think the suspension check is necessary, as even if the target is
suspended we must still be at a safepoint or in a handshake with it. Makes me
wonder if we used to allow a racy stacktrace operation on a suspended thread,
assuming it would remain suspended?
1268 oop thread_oop = jt->threadObj();
1269
1270 if (!jt->is_exiting() && (jt->threadObj() != NULL)) {
You can use thread_oop in line 1270.
1272 _collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread,
thread_oop),
1273 jt, thread_oop);
It is frustrating that this entire call chain started with a jthread reference,
which we converted to a JavaThread, only to eventually need to convert it back
to a jthread! I think there is some scope for simplification here but not as
part of this change.
1271 ResourceMark rm;
IIUC at this point the _calling_thread is the current thread, so we can use:
ResourceMark rm(_calling_thread);
---
Please add @bug lines to the tests.
I'm still pondering the test logic but wanted to send this now.
Thanks,
David
-
VM_GetThreadListStackTrace (for GetThreadListStackTraces) and
VM_GetAllStackTraces (for GetAllStackTraces) have inherited
VM_GetMultipleStackTraces VM operation which provides the feature to generate
jvmtiStackInfo. I modified VM_GetMultipleStackTraces to a normal C++ class to
share with Hands
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
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
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Hi Yasumasa,
On 30/06/2020 10:05 am, Yasumasa Suenaga wrote:
Hi David, Serguei,
I updated webrev for 8242428. Could you review again?
This change migrate to use direct handshake for GetStackTrace() and
GetThreadListStackTraces() (when thread_count == 1).
http://cr.openjdk.java.net/~ysuenaga/JDK-8242428/webrev.01/
This looks really good now! I only have a few nits below. There is one
thing I don't like about it but it requires a change to the main
Handshake logic to address - in JvmtiEnv::GetThreadListStackTraces you
have to create a ThreadsListHandle to convert the jthread to a
JavaThread, but then the Handshake::execute_direct creates another
ThreadsListHandle internally. That's a waste. I will discuss with Robbin
and file a RFE to have an overload of execute_direct that takes an
existing TLH. Actually it's worse than that because we have another TLH
in use at the entry point for the JVMTI functions, so I think there may
be some scope for simplifying the use of TLH instances - future RFE.
---
src/hotspot/share/prims/jvmtiEnvBase.hpp
451 GetStackTraceClosure(JvmtiEnv *env, jint start_depth, jint
max_count,
452jvmtiFrameInfo* frame_buffer, jint* count_ptr)
453 : HandshakeClosure("GetStackTrace"),
454 _env(env), _start_depth(start_depth), _max_count(max_count),
455 _frame_buffer(frame_buffer), _count_ptr(count_ptr),
456 _result(JVMTI_ERROR_THREAD_NOT_ALIVE) {
Nit: can you do one initializer per line please.
This looks wrong:
466 class MultipleStackTracesCollector : public StackObj {
498 class VM_GetAllStackTraces : public VM_Operation {
499 private:
500 JavaThread *_calling_thread;
501 jint _final_thread_count;
502 MultipleStackTracesCollector _collector;
You can't have a StackObj as a member of another class like that as it
may not be on the stack. I think MultipleStackTracesCollector should not
extend any allocation class, and should always be embedded directly in
another class.
481 MultipleStackTracesCollector(JvmtiEnv *env, jint max_frame_count) {
482 _env = env;
483 _max_frame_count = max_frame_count;
484 _frame_count_total = 0;
485 _head = NULL;
486 _stack_info = NULL;
487 _result = JVMTI_ERROR_NONE;
488 }
As you are touching this can you change it to use an initializer list as
you did for the HandshakeClosure, and please keep one item per line.
---
src/hotspot/share/prims/jvmtiEnvBase.cpp
820 assert(SafepointSynchronize::is_at_safepoint() ||
821 java_thread->is_thread_fully_suspended(false, &debug_bits) ||
822 current_thread == java_thread->active_handshaker(),
823 "at safepoint / handshake or target thread is suspended");
I don't think the suspension check is necessary, as even if the target
is suspended we must still be at a safepoint or in a handshake with it.
Makes me wonder if we used to allow a racy stacktrace operation on a
suspended thread, assuming it would remain suspended?
1268 oop thread_oop = jt->threadObj();
1269
1270 if (!jt->is_exiting() && (jt->threadObj() != NULL)) {
You can use thread_oop in line 1270.
1272
_collector.fill_frames((jthread)JNIHandles::make_local(_calling_thread,
thread_oop),
1273jt, thread_oop);
It is frustrating that this entire call chain started with a jthread
reference, which we converted to a JavaThread, only to eventually need
to convert it back to a jthread! I think there is some scope for
simplification here but not as part of this change.
1271 ResourceMark rm;
IIUC at this point the _calling_thread is the current thread, so we can use:
ResourceMark rm(_calling_thread);
---
Please add @bug lines to the tests.
I'm still pondering the test logic but wanted to send this now.
Thanks,
David
-
VM_GetThreadListStackTrace (for GetThreadListStackTraces) and
VM_GetAllStackTraces (for GetAllStackTraces) have inherited
VM_GetMultipleStackTraces VM operation which provides the feature to
generate jvmtiStackInfo. I modified VM_GetMultipleStackTraces to a
normal C++ class to share with HandshakeClosure for
GetThreadListStackTraces (GetSingleStackTraceClosure).
Also I added new testcases which test GetThreadListStackTraces() with
thread_count == 1 and with all threads.
This change has been tested in serviceability/jvmti serviceability/jdwp
vmTestbase/nsk/jvmti vmTestbase/nsk/jdi vmTestbase/nsk/jdwp.
Thanks,
Yasumasa
On 2020/06/24 15: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
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
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 {
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
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
Hi Robbin,
We decided to separate thread operation and frame operation.
I've posted review request for thread operation. Could you review it?
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.
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.
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
Re: RFR: 8242428: JVMTI thread operations should use Thread-Local Handshake
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
Re: RFR (S) 8247615: Initialize the bytes left for the heap sampler
Looks good to me, but of course I'm not qualified to review.
On Mon, Jun 29, 2020 at 3:26 PM Jean Christophe Beyler
wrote:
>
> Agreed; missed a hg qrefresh; link is now updated:
> http://cr.openjdk.java.net/~jcbeyler/8247615/webrev.01/
>
> :)
> Jc
>
> On Mon, Jun 29, 2020 at 2:28 PM Man Cao wrote:
>>
>> Looks good.
>>
>> > though adding the change that Man wants might make it more flaky so I
>> > added your numThreads / 2 in case
>> I don't see the "numThreads / 2" in webrev.01 though. No need for a webrev
>> for this fix.
>>
>> -Man
>>
>>
>> On Mon, Jun 29, 2020 at 1:10 PM Jean Christophe Beyler
>> wrote:
>>>
>>> Hi all,
>>>
>>> Sorry it took time to get back to this; could I get a new review from:
>>> http://cr.openjdk.java.net/~jcbeyler/8247615/webrev.01/
>>>
>>> The bug is here:
>>> https://bugs.openjdk.java.net/browse/JDK-8247615
>>>
>>> Note, this passed the submit repo testing.
>>>
>>> Thanks and have a great day!
>>> Jc
>>>
>>> Ps: explicit inlined Acks/Done are below:
>>>
>>> Sorry it took time to get back to this:
>>> @Martin:
>>>- done the typo
>>>- about the sampling test: No you won't get samples due to how the
>>> system is done, since we know we only will be allocating one object for the
>>> thread, it dies out before a sample is required... though adding the change
>>> that Man wants might make it more flaky so I added your numThreads / 2 in
>>> case
>>> - done for the always in the description
>>>
>>>
>>> On Thu, Jun 25, 2020 at 6:54 PM Derek Thomson wrote:
> It could also avoid the problem where every thread deterministically
> allocates the same object at 512K, although this is unlikely.
I've recently discovered that with certain server frameworks that this
actually becomes quite likely! So I'd strongly recommend using
pick_next_sample.
>>>
>>>
>>> Ack, done :)
>>>
On Thu, Jun 25, 2020 at 4:56 PM Man Cao wrote:
>
> Thanks for fixing this!
>
> > 53 ThreadHeapSampler() : _bytes_until_sample(get_sampling_interval()) {
>
> Does this work better? (It has to be done after the initialization of
> _rnd.)
> _bytes_until_sample = pick_next_sample();
>
> It could avoid completely missing to sample the first 512K allocation.
> It could also avoid the problem where every thread
>>>
>>>
>>> Done.
>>>
>>>
>
> deterministically allocates the same object at 512K, although this is
> unlikely.
>
> -Man
>>>
>>>
>>>
>>> --
>>>
>>> Thanks,
>>> Jc
>
>
>
> --
>
> Thanks,
> Jc
