JDK-8185003 JMX: Add a version of ThreadMXBean.dumpAllThreads with a maxDepth argument

[Hohensee, Paul]

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" <serviceability-dev-bounces at openjdk.java.net on behalf of ujwal.vangapally at oracle.com> 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.
    >>>
    >>
    >