2009/9/3 Rémi Forax <[email protected]>: > 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
