What do people think of the idea of extending this RFE to include adding versions of dumpAllThreads that are duals of the existing getThreadInfo(long[], …) methods? And also add
getThreadInfo(long[] ids, boolean lockedMonitors, boolean lockedSynchronizers, int maxDepth) as the dual of the proposed dumpAllThreads(boolean lockedMonitors, boolean lockedSynchronizers, int maxDepth) Paul On 8/3/17, 9:44 AM, "serviceability-dev on behalf of Ujwal Vangapally" <[email protected] on behalf of [email protected]> wrote: Thanks for the review Roger, please see my comments inline. On 8/3/2017 8:23 PM, Roger Riggs wrote: > Hi Ujwal, > > (Reviewer, but not specifically servicability). > > Comments, > > java/lang/management/ThreadMXBean.java: > > 809: It may be useful to state that the behavior is the same as {@link > #dumpAllThreads} except that the depth is limited. > > 828: Do not duplicate specification of the meaning of maxDepth = > MAX_VALUE, either put the entire spec > in the @param or in the description. Duplication creates an > opportunity for disagreement or a maintenance issue when updating. > > 833: terminate the {@code maxDepth} before "is negative". > > As Erik comments, there should be a space after "," everywhere; both > to be consistent with existing style and the style guide. will make changes to doc as suggested. > > sun/management/ThreadImpl.java: > > 484: Since the native code is going to check maxDepth, why is it > checked here also? > -or- remove the check in native As maxDepth value should not be negative for dumpAllThreads sun/management/ThreadImpl.java: 484: throws IllegalArgumentException. Check on native side can be removed but having it might help in enforcing the convention of using only -1 for dumping all the stack frames otherwise any negative number can be passed to dump all the frames. > 490: This could be simpler to not translate maxDepth to -1, > Here and in the native code, passing MAX_INTEGER would dump the > entire stack, there is no need to special case it > > services/threadService.cpp: > 565: checking count >= maxDepth is a sufficient test if to dump all > frames, passing MAX_INTEGER is used > and it will dump zero should a negative number get this far. > currently getThreadInfo method accepts maxDepth argument and it uses the translation of maxDepth to "-1" as it's implementation detail for printing all stack frames, As getThreadInfo also uses services/threadService.cpp: 565: (dump_stack_at_safepoint) expecting it to print all stack trace elements when maxDepth is given as "-1". I followed same convention so that it will not affect getThreadInfo method with maxDepth argument. > Thanks, Roger > Thanks, Ujwal > On 8/3/2017 10:18 AM, Ujwal Vangapally wrote: >> kindly use the below link for accessing webrev >> >> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.01/ >> >> previously shared link is no longer accessible hence providing this >> new link. >> >> Thanks, >> >> Ujwal. >> >> >> On 8/3/2017 3:08 PM, Ujwal Vangapally wrote: >>> Hi, >>> >>> kindly review the changes made. >>> >>> https://bugs.openjdk.java.net/browse/JDK-8185003 >>> >>> webrev: >>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.00/ >>> >>> CSR: https://bugs.openjdk.java.net/browse/JDK-8185705 >>> >>> Thanks, >>> >>> Ujwal. >>> >> >
