Pls review 6173675/7003271

[Mandy Chung]

  On 01/04/11 13:00, Paul Hohensee wrote:
> These two rfes implement per-thread approximate memory allocation 
> tracking.
> 6173675 also adds multi-thread-id versions of getThreadCpuTime and 
> getThreadUserTime.
>
> *6173675 M&M: approximate memory allocation rate/amount per thread 
> <http://monaco.sfbay.sun.com/detail.jsf?cr=6173675>
> **7003271 Hotspot should track cumulative Java heap bytes allocated on 
> a per-thread basis <http://monaco.sfbay.sun.com/detail.jsf?cr=7003271>
>
> *6173675 is the library part, while 7003271 is the Hotspot part.
>
> Webrevs here
>
> http://cr.openjdk.java.net/~phh/6173675/webrev.00/

I reviewed the jdk change.

Happy new year! Should the copyright year be 2011?

Can you include the unit tests in the webrev?

sun/management/ThreadImpl.java
line 153-155: the spec allows the given ids array of zero-length.   The 
check should not be added.   You may want to add a check in line 175 to 
return infos if ids.length == 0.

Thanks for doing the refactoring.  I think the check calling 
isThreadCpuTimeEnabled() should be included in the 
verifyCurrentThreadCpuTime and verifyThreadCpuTime methods.  How about 
having these 2 methods to return a boolean; i.e. return 
isThreadCpuTimeEnabled().

boolean verifyCurrentThreadCpuTime()
boolean verifyThreadCpuTime(long[] ids)


The callers to the verifyThreadCpuTime(long[]) method will do its own 
allocation of the resulting long[]. line 255-259 can be refactored as a 
separate method (which can also be called by the getThreadAllocatedBytes 
method.

Similarly, the verifyThreadAllocatedMemory method can return a boolean 
(isThreadAllocatedMemoryEnabled()) and have the caller to construct the 
resulting array.

Mandy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20110105/f2838b10/attachment.html