Dear Mandy & Peter, I will test your patch asap (+ benchmark)
Few comments below: > Webrev at: > http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8010309/webrev.00/ > > Please let me know if you don't agree with these changes or you find any > performance issue. > > I have retained the part in webrev.08 that only defines loggerProxy and > javaLoggerProxy. I don't think the new defaultLoggerProxy variable is > needed. I prefer to use Class.forName to force load JavaLoggerProxy class > to make very explicit that we don't want the class be initialized. > JavaLoggerProxy.<clinit> calls LoggingProxy.parseLevel unconditionally that > will throw an AssertionError if java.util.logging is not available. If > JavaLoggerProxy class is initialized, j.u.logging should be present and I > want to catch error if otherwise. Also Class.forName will load its > superclass LoggerProxy. > I think it leads to early initialization of JUL Levels (when JUL is maybe not available or initialized) from PlatformLogger static initialization: // initialize javaLevel fields for mapping from Level enum -> j.u.l.Level object static { for (Level level : Level.values()) { level.javaLevel = LoggingSupport.parseLevel(level.name()); } } I thought Level.javaLevel should only be defined ONCE JUL is available i.e. once PlatformLogger.redirectPlatformLoggers() is called. I propose to move the javaLevel initialization in this method: public static synchronized void redirectPlatformLoggers() { if (loggingEnabled || !LoggingSupport.isAvailable()) return; // Resolve JUL Levels ONLY when JUL is initialized by application code: for (Level level : Level.values()) { level.javaLevel = LoggingSupport.parseLevel(level.name()); } ... } > > As for the PlatformLogger.Level enums, having the members to be defined in > ascending order is fragile. It'd be more reliable to specify the value in > the constructor so that it can be used as the isLoggable comparsion in the > same way as in the Level implementation. > Good idea. > > On 3/27/2013 9:44 AM, Peter Levart wrote: > > Hi Mandy, Laurent, > > It turns out that the API change (change of type for level parameter int > -> enum Level) is entirely source-compatible. The tip of JDK8 (tl repo) > builds without a problem. So no-one is currently using > PlatformLogger.getLevel() method and assigning it to a variable or such... > > > Great thanks. That is what I expect too. > If getLevel() is useless, why keep it in this proprietary API ? (only for test purposes) ? > > > Here's the webrev for this change: > > > http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.enumapi.01/index.html > > Besides doing the replacement of type and renaming and removing of the > unneeded stuff, I also did some re-arrangements: > > - introduced a common abstract superclass for both types of LoggerProxys > (DefaultLoggerProxy, JavaLoggerProxy), since JavaLoggerProxy does not need > the fields of DefaultLoggerProxy and now both concrete subclasses can be > declared final (makes JIT even more happy). Also the abstract LoggerProxy > could host some common logic (see below about formatting)... > > > That's a good idea. > > > - DefaultLoggerProxy's levelValue/effectiveLevel were given names just > the opposite of their meaning. I aligned them with terminology used in > j.u.l.Logger and renamed levelValue to plain level. > > > Swapping these 2 variables makes sense. > > > - introduced private method DefaultLoggerProxy.deriveEffectiveLevel(level) > that currently just returns defaultLevel (INFO) when given null. > > > Ok. > > > I think with a little more effort, it could be possible to emulate the > behaviour of j.u.l.Logger which inherits from 1st parent logger that has > non-null level assigned. Of course with all the caching and invalidation... > > > The only way to change the level to a non-default one is via the logging > configuration and enable logging. That's why there is no need for > DefaultLoggerProxy to inherit level from its parent logger. Also there is > no parent logger hierarchy maintained in DefaultLoggerProxy. > > > - instead of static final DefaultLoggerProxy.defaultStream I created a > private static method outputStream() that returns System.err. To accomodate > for the situation when System.err is changed dynamically. > > > Ok. > > > - fixed the JavaLoggerProxy.isEnabled() method. Original code was: > > 532 boolean isEnabled() { 533 Object level = > LoggingSupport.getLevel(javaLogger); 534 return level == null || > level.equals(levelObjects.get(OFF)) == false; > 535 } > > > If 'level' is null then it can be that 1st parent that has a non-nul level > is "OFF". I think that in such situation all the children that don't > override the level should be disabled too. The following does exactly that: > > 597 boolean isEnabled() { 598 return > LoggingSupport.isLoggable(javaLogger, Level.OFF.javaLevel); > 599 } > > > > That is right. Good catch. > > > That's all for 1st rev. Besides the effective level inheritance, the > following method in JavaLoggerProxy also caught my eye: > > void doLog(Level level, String msg, Object... params) { > if (!isLoggable(level)) { > return; > } > // only pass String objects to the j.u.l.Logger which may > // be created by untrusted code > int len = (params != null) ? params.length : 0; > Object[] sparams = new String[len]; > for (int i = 0; i < len; i++) { > sparams [i] = String.valueOf(params[i]); > } > LoggingSupport.log(javaLogger, level.javaLevel, msg, sparams); > } > > I think this could be improved if the DefaultLoggerProxy.formatMessage() > is used instead of turning each parameter into a String. The method could > be moved up to abstract LoggerProxy and used in both implementations so > that common formatting is applied regardless of back-end used. > > > Let's do this in a separate clean up as it's better to keep 8010309 focus > on performance improvement (although we have mixed this bag with some > renaming). > I disagree Peter: JUL has its own formatting code: patterns ... and more efficient than DefaultLoggerProxy.formatMessage(). The good question relies in the comment: why convert object args into String early as JUL can do formatting / conversion? What does mean: // only pass String objects to the j.u.l.Logger which may // be created by untrusted code ? to avoid security issues ? Laurent
