8010309 : PlatformLogger: isLoggable performance / waste due to HashMap<Integer, Level> leads to Integer allocations (boxing)

[Laurent Bourgès]

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