Thank you for review, David!
Serguei
On 10/1/19 19:27, David Holmes wrote:
Looks good.
Thanks,
David
On 2/10/2019 9:57 am, serguei.spit...@oracle.com wrote:
Hi David,
Yes, this is another place to fix the same typo, thanks.
It has to be results[i] instead of err.
I'll update the webrev in place.
Thanks,
Serguei
On 10/1/19 4:00 PM, David Holmes wrote:
Hi Serguei,
Shouldn't this:
80 for (int i = 0; i < threadsCount; i++) {
81 LOG(" thread #%d: (%d)", i, (int)results[i]);
82 check_jvmti_status(jni, err, "suspendTestedThreads: error
in SuspendThreadList");
also be testing results[i] rather than err? Or do you need to test
err independently, as well as each result?
David
On 2/10/2019 8:33 am, serguei.spit...@oracle.com wrote:
Alex, Chris and David,
The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.3/
This version changes are:
- the "first" and "last" are passed to the test to set the
suspenderIndex
- fixed typo at line 120
- the ThreadToSuspend.run() loop is simplified
- fixed typo in check_jvmti_status in the loop of
resumeTestedThreads()
Probably, just one sanity check would be enough.
Thanks,
Serguei
On 10/1/19 2:20 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
On 10/1/19 12:46 PM, Chris Plummer wrote:
Hi Serguei,
If someone changes THREADS_COUNT, then SuspenderIndex would no
longer do what we want. I suggest passing in something like
"first" and "last".
Okay, I'll update it this way.
120 * - main thread registers tested threads withing the native
agent library
Should be "within".
Good catch, will fix it.
I think you should add a comment above all the "n" and "i" logic
explaining what it is for, although TBH, I don't see how this is
making the method "hot" and therefore trigger compilation. The
loop alone should be enough for that. In fact I would think the
more iterations through the loop, the sooner it would be
compiled, and this extra logic just slows down the iteration rate.
Agreed.
I'll try to simplify it.
Thanks,
Serguei
thanks,
Chris
On 9/30/19 10:45 PM, serguei.spit...@oracle.com wrote:
Hi Chris, David and Alex,
The updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.2/
It includes the changes:
- added a general comment explaining the test logic suggested
by David
- setAllThreadsReady() is static now, the obsolete comment
before it is deleted
- now the test is run twice: with the suspender thread first
and last in the list
- removed the local variable "success" in the run() method
- several native agent methods return "void" now
Thanks,
Serguei
On 9/30/19 13:49, Chris Plummer wrote:
On 9/30/19 1:30 PM, serguei.spit...@oracle.com wrote:
On 9/30/19 13:25, Chris Plummer wrote:
On 9/30/19 1:21 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
Thank you for reviewing this!
On 9/28/19 12:33, Chris Plummer wrote:
Hi Serguei,
Overall looks good. A few questions:
I don't understand the need for all the 'i' and 'n'
theatrics in the shouldFinish loop. Can you explain and
also add a comment?
I used this part from one of the old SuspendThreadList nsk
tests in the vmTestbase.
As I understand it, the point was to make sure the JVMTI
SuspendThreadList works well
wen the top frames executed on tested threads have been
compiled.
These code is needed to make the run() method hot.
I can add a comment if you think it is not clear.
Is this comment right?
193 // set thread is not ready again
194 public void setAllThreadsReady() {
195 allThreadsReady = true;
196 }
Nice catch.
The comment is not needed anymore.
Is a leftover from previous test version.
Also, shouldn't "setAllThreadsReady()" be static?
Right. It has to be static. Will fix it.
Do you think it would be useful to also run the test with
the last thread in the list being the suspender thread?
I'm not sure it is worth to do it.
It'd add more complexity into the test.
We could try to make the suspender thread to be random though.
I don't think random is good. Makes it hard to reproduce an
issue if it turns up. I was thinking just rerun the test for
each possible suspender.
Good idea to rerun the test and pass the suspender thread
index in the arguments.
Do you think, two runs would be good enough or we also need an
index somewhere in the middle?
Maybe just first and last indices would be good. I'm not sure
something in the middle helps any.
Chris
Thanks,
Serguei
thanks,
Chris
Thanks,
Serguei
thanks,
Chris
On 9/27/19 6:25 PM, serguei.spit...@oracle.com wrote:
Please, review fix for test enhancement:
https://bugs.openjdk.java.net/browse/JDK-8231595
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.1/
Summary:
New test is a coverage for the JVMTI bug:
https://bugs.openjdk.java.net/browse/JDK-8217762
SuspendThreadList won't work correctly if the
current thread is not last in the list
It provides a prove the bug JDK-8217762 does not exist
as the test is passed with the current implementation.
Thanks,
Serguei
