<AWT Dev> <Swing Dev> sun.awt.X11 logs still using String + (waste)

[Laurent Bourgès]

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.chung at 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-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/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
>
>
>