Review Request for 6878481: Add performance counters in the JDK

[Andrew John Hughes]

2009/9/3 Rémi Forax <forax at univ-mlv.fr>:
> 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
>

The use of synchronized at present seems flawed as only the set
methods are protected and not the get.  So lb.get could be called
while in an lb.put call.  I don't see the reason for using a
LongBuffer either, as only index 0 is ever used.  Why not use an
AtomicLong?

What is the purpose of d3dAvailable?  I don't know this piece of code
so I'm not sure what it's counting, but it will presumably be zero
forever on non-Windows systems.
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8