-
Notifications
You must be signed in to change notification settings - Fork 729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new ThreadMXBean APIs #1471
Conversation
Tested in a personal build, but results are no longer available. Would a committer kindly run a test build? Ran the new tests manually with logging enabled: ===============================================
|
* <li><code>lockedSynchronizers</code> is <code>true</code> | ||
* but a call to {@link #isSynchronizerUsageSupported()} would | ||
* result in a <code>false</code> value | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</ul>
is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add @since 10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@Override | ||
public ThreadInfo[] getThreadInfo(long[] ids, boolean lockedMonitors, | ||
boolean lockedSynchronizers, int maxDepth) { | ||
// K0662 = maxDepth must not be negative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These external message declarations are supposed to be preprocessor commands, not comments. Please restore the appropriate preprocessor format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
return this.getMultiThreadInfoImpl(ids, Integer.MAX_VALUE, | ||
lockedMonitors, lockedSynchronizers); | ||
} | ||
boolean lockedSynchronizers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original file uses tabs, but the changes use spaces to indent. Please make the new code consistent with the rest of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
52e8fef
to
a256e18
Compare
* but a call to {@link #isSynchronizerUsageSupported()} would | ||
* result in a <code>false</code> value | ||
* </ul> | ||
* @since 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some unnecessary indenting here and in the next method as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was copied that from the other versions. I cleaned it up and added missing s.
Do you want me to clean up the originals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to the extra tab before @since 10
, which is still there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I will push the changes with changes from @llxia 's review.
continue; | ||
} | ||
int requestedDepth = requestedDepthTemp; | ||
if (id != myId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this conditional always be true, since myId is not added to expectedDepths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't always true, but a bug fix made it a tautology. Refactored and fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now:
a) check all threads against the maximum depth at the top of the loop.
b) omit the check against myId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see the check against myId at L178.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Missed that.
if (id != myId) { | ||
checkDepth(requestedDepth, maxDepth, trace, "dumpAllThreads"); | ||
} else { | ||
Assert.assertTrue(trace.length <= maxDepth, "dumpAllThreads: Wrong stack size when maxDepth = "+maxDepth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this code compile? trace
is not declared in this scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's declared on line 159.
waitObject.notifyAll(); | ||
} | ||
for (Thread t: myThreads) { | ||
if (Thread.currentThread().equals(t)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be any point in adding the current thread to myThreads
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Fixed.
Assert.assertEquals(trace.length, maxDepth, msg+": Wrong stack size when requested depth > maxDepth = "+maxDepth); | ||
} else { | ||
Assert.assertTrue(trace.length <= maxDepth, msg+": Stack exceeds maxDepth = "+maxDepth); | ||
Assert.assertTrue(trace.length >= requestedDepth, msg+": Stack less than minimum expected"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would ==
work here instead of >=
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that.
The actual depth may be greater than the expected depth due to internal calls within wait().
@llxia please review the tests |
a256e18
to
fa1cc72
Compare
</levels> | ||
<groups> | ||
<group>functional</group> | ||
</groups> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have subset? Is this test for java10?
<subsets>
<subset>SE100</subset>
</subsets>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks.
87a96ff
to
1a1f463
Compare
Add Add ThreadMXBean.dumpAllThreads(..., int maxDepth) and getThreadInfo(..., int maxDepth) APIs for Java 10. Add tests for the new APIs, including stack frames with method handles and lambdas. Fix incorrect copyright. Signed-off-by: Peter Bain <[email protected]>
1a1f463
to
ed87a68
Compare
Jenkins test sanity plinux jdk9 |
Add Add ThreadMXBean.dumpAllThreads(..., int maxDepth) and getThreadInfo(..., int maxDepth) APIs
for Java 10.
Add tests for the new APIs, including stack frames with method handles and lambdas.
Fix incorrect copyright.
Fixes #577
Signed-off-by: Peter Bain [email protected]