Anthony or Mandy, Could you ask JMX / Security groups for me to review my patch ?
I am currently not registered to these mailing lists. Do you ask me to split the patch in two part: PlatformLogger on one side and Logger on the other side ? Laurent 2013/4/9 Mandy Chung <mandy.ch...@oracle.com> > On 4/9/13 12:37 AM, Laurent Bourgès wrote: > > Mandy, > > first I would like to have the given patch applied to OpenJDK 8 (+ JDK7u) > as it fixes many problems: > - string concatenations > - object creations (Object[] or other objects given as params) > - method calls in log statements > > This is the patch you refer to: > http://jmmc.fr/~bourgesl/share/webrev-8010297.3/ > > I agree that we should separate the fix to reduce the memory usage from > the fix to convert JUL to PlatformLogger. I skimmed on your patch - > awt/swing uses PlatformLogger and your change looks fine. You have also > got Anthony's approval. Other component security, jmx, snmp, etc are using > JUL. I would suggest to fix the use of PlatformLogger in your first patch > and AWT/Swing is the main area that 8010297 is concerned about so that you > can resolve 8010297 soon. > > If you want to move ahead to fix use of JUL, you can send your patch to > serviceability-...@openjdk.java.net and security-...@openjdk.java.net for > the other areas to review and sponsor. > > Hope this helps. > Mandy > > > In a second step, I can help somebody migrating JUL usages to > PlatformLogger but it requires PlatformLogger API changes (logp to give > class and method names)... > > Other comments below: > >> >> Peter's idea is a good one to add a couple of convenient methods to take >> Object parameters that will avoid the creation of array instances. I'd >> also like to know what variants being used in the jdk to determine the pros >> and cons of the alternatives (this issue also applies to java.util.logging). >> > > 50% of given object parameters are created via method calls so it will not > be enough. > > Moreover, I prefer having always isLoggable() calls to avoid me looking > and understanding special cases (more than 2 args or no string concat ...); > besides, it would be easier to implement code checks testing missing > isLoggable() calls vs conditional and specific checks (string concat, more > then 2 args, method calls ...) > > Finally, I prefer having shorter and clearer method names like isFine(), > isFinest() instead of isLoggable(PlatformLogger.FINE) that is quite "ugly" > ... > > >> It was intentional to have PlatformLogger define only the useful methods >> necessary for jdk use. The goal of PlatformLogger was to provide a >> light-weight utility for jdk to log debugging messages and eliminate the >> dependency to java.util.logging and avoid the unnecessary j.u.logging >> initialization (as disabled by default). It was not a goal for >> PlatformLogger to be an exact mirror as java.util.logging.Logger. My >> advice is to tune PlatformLogger to provide API that helps the platform >> implementation while avoid bloating the API. >> > > Agreed. > > Maybe JUL Logger.logp(classname, methodname) usages should be rewritten to > use PlatformLogger.getCallerInfo() instead: > // Returns the caller's class and method's name; best effort > // if cannot infer, return the logger's name. > private String getCallerInfo() { > String sourceClassName = null; > String sourceMethodName = null; > > * JavaLangAccess access = SharedSecrets.getJavaLangAccess(); > * Throwable throwable = new Throwable(); > int depth = access.getStackTraceDepth(throwable); > > String logClassName = "sun.util.logging.PlatformLogger"; > boolean lookingForLogger = true; > for (int ix = 0; ix < depth; ix++) { > // Calling getStackTraceElement directly prevents the VM > // from paying the cost of building the entire stack frame. > StackTraceElement frame = > access.getStackTraceElement(throwable, ix); > String cname = frame.getClassName(); > if (lookingForLogger) { > // Skip all frames until we have found the first > logger frame. > if (cname.equals(logClassName)) { > lookingForLogger = false; > } > } else { > if (!cname.equals(logClassName)) { > // We've found the relevant frame. > sourceClassName = cname; > sourceMethodName = frame.getMethodName(); > break; > } > } > } > > if (sourceClassName != null) { > return sourceClassName + " " + sourceMethodName; > } else { > return name; > } > } > } > > >> >> Since you are touching some jdk files that use java.util.logging, would >> you like to contribute to convert them to use PlatformLogger: >> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7054233 >> >> It's perfectly fine to convert only some of them if not all. >> > > I want to help but as you said it should be done in collaboration because > it requires API changes (JMX, RMI ...) but I prefer after this patch is > reviewed and submitted and in coming weeks. > > >> >> Jim Gish is the owner of logging and will check with him if he has cycles >> to work with you on this. >> > > Great. > > Cheers, > Laurent > > >
