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

[Roger Riggs]

Hi Ujwal,


On 8/3/2017 12:44 PM, Ujwal Vangapally 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.
There is no need for a separate indication of dumping all stack frames; 
a count of MAX_INTEGER
will have the same effect.

Even the specification in ThreadMXBean does not need to call out the 
special value for maxDepth
  as MAX_INTEGER as a special value.  It just makes the spec more 
complicated.
>> 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,
Still, with a sufficiently large maxDepth it is indistinguishable from 
the 'all frames' sentinel value.
The code is just more complicated than necessary as a result.

> 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".
Yes, it has the same effect as a very large maxDepth.
> I followed same convention so that it will not affect getThreadInfo 
> method with maxDepth argument.
fine, no need to change the native code, but the same result is achieved 
at the java level without
the adding the complexity in the implementation of ThreadImpl:489.

Roger

>> 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.
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20170803/b4f00826/attachment.html>