Review Request for 6878481: Add performance counters in the JDK

[Rémi Forax]

Le 03/09/2009 10:54, Alan Bateman a écrit :
> Mandy Chung wrote:
>> This is related to 6857194: Add hotspot new perf counters to aid 
>> class loading performance measurement.
>>
>> It's useful to add performance counters in the library code so that 
>> perf data from the JDK and VM can be collected and output in a 
>> unified way (written in the jvmstat shared memory buffer).  I add a 
>> simple sun.misc.PerfCounter class to maintain the list of perf 
>> counters for the library to use.  This fix only instruments the class 
>> loading and jar/zip to collect simple basic metrics.   Additional 
>> perf counters will be added in the future.
>>
>> Webrev:
>>     http://cr.openjdk.java.net/~mchung/6878481/webrev.00/
>>
>> Thanks
>> Mandy
> It's good to see the use of the instrumentation buffer extended to the 
> libraries.
>
> I'm not sure how sun.misc.Perf behaves when running with 
> -XX:-UsePerfData. You've probably checked this anyway, but just in 
> case...
>
> It would be good to include a comment to define the meaning of each of 
> the counters. For example, in ClassLoader.loadClass, it's not clear if 
> you mean to include the time to check if the class has already been 
> loaded (just on that one, if the lookup is not meant to be included 
> then it would reduce the calls to nanoTimes a bit).
>
> Is it necessary to have separate counters for zip and JAR? If I'm 
> reading this correctly, then zipFiles and jarFiles will both be 
> incremented when a JAR file is opened - is that right?
>
> I agree with Rémi's comment that perf, name, and lb can be final.
>
> Would addElapsedTime(start) be better named as 
> addElapsedTimeFrom(start) to make it clear that it doesn't add "start" 
> to the elapsed time, but rather the elapsed time from the given 
> starting time?
>
> This might sound crazy, but do the updates need to be synchronized? I 
> don't think there is any synchronization in the VM. I agree with 
> David's concern about the overhead of the calls to nanoTimes (which 
> I'm sure you are measuring) but for the synchronization there are 
> places where both a counter and an elapsed time are updated.

Hi Alan,
Classloading is now done in parallele, zip and jar can be loaded by 
different threads.

"for the synchronization there are places where both a counter and an 
elapsed time are updated"
=> the problem is that these synchronisations are done on two different 
monitors.
       I don't see how to remove them easily.

Furthermore I think that all synchronized can not be easily replaced by 
some atomics operations
(unsafe.put* or unsafe.compareAndSwap) because long in a ByteBuffer 
aren't volatile.
Perhaps with the Doug Lea's Fences (scheduled to be introduced in jdk7) 
but I'm not sure.

>
> -Alan.
Rémi