sun.awt.X11 logs still using String + (waste) (original) (raw)

Mandy Chung mandy.chung at oracle.com
Tue Apr 9 21:10:40 UTC 2013

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/ <http://jmmc.fr/%7Ebourgesl/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-dev at openjdk.java.net and security-dev at 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/viewbug.do?bugid=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

More information about the core-libs-dev mailing list