Review Request for 6878481: Add performance counters in the JDK

[Mandy Chung]

Alan Bateman wrote:
> 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...
Adding -XX:-UsePerfData option works fine but I am yet to understand the 
logics since this should be different than the case with a small 
PerfDataMemorySize where the perf counters are still created but in the 
C heap.  I'm looking into the hotspot perfdata implementation further.
>
> 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).
>
As David points out the high number of calls to System.nanoTime, I 
should refine the fix not to include the lookup time.  I'll update the 
fix and also describes the meaning of each counter.
> 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?
>
That's right, both zipFiles and jarFiles counters are incremented. 
> I agree with Rémi's comment that perf, name, and lb can be final.
>
Agreed.
> 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?
>
Ok.
> 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.
>
Yes, I ran the server benchmarks on Windows XP that shows negligible 
overhead.  I have to rerun the solaris-i586 measurement as I got some 
high standard deviation.  I'm composing my reply to David's email.

With the synchronization, we might miss some updates.  It's a tradeoff 
between correctness and performance overhead (I have to find out the 
reasons why it was decided not to have synchronization in the hotspot 
implementation).  Is the counter update a fast operation - updating the 
direct buffer?  I think so and thus this should not cause high lock 
contention on the PerfCounter.

Thanks
Mandy
> -Alan.