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.
