Review request: 8011380: FX dependency on PlatformLogger broken

Fri Apr 5 08:55:14 UTC 2013

Mandy,

I would like to add few performance advices in the PlatformLogger Javadoc:
"
NOTE: For performance reasons, PlatformLogger usages should take care of
avoiding useless / wasted object creation and method calls related to *
disabled* log statements:
Always use isLoggable(level) wrapping logs at levels (FINEST, FINER, FINE,
CONFIG):

Bad practices:
- string concat:
    log.fine("message" + value); // means
StringBuilder(message).append(String.valueOf(value)).toString(): 2 objects
created and value.toString() called
- varags:
    log.fine("message {0}", this); // create an Object[]

Best practices:
if (log.isLoggable(PlatformLogger.FINE) {
    log.fine("message" + value);
}

if (log.isLoggable(PlatformLogger.FINE) {
    log.fine("message {0}", this);
}
"

What is your opinion ?

Thanks for the given explanations and I hope that this patch will be
submitted soon to JDK8 repository.

Laurent

2013/4/4 Mandy Chung <mandy.chung at oracle.com>

> Alan - can you review this change?
>
> I have changed Level.valueOf(int) to return the nearest Level as Peter
> suggests using binary search:
>    http://cr.openjdk.java.net/~**mchung/jdk8/webrevs/8011380/**webrev.01/<http://cr.openjdk.java.net/%7Emchung/jdk8/webrevs/8011380/webrev.01/>
>
> I want to push the changeset tomorrow since we need this fix before the TL
> integration.
>
> Several questions were brought up and I'm answering them in one shot:
> 1. The PlatformLogger static final fields have to retain the same since
> existing code can call:
>     int level = PlatformLogger.FINE;
>     logger.setLevel(level);
>
> There is also native code accessing PlatformLogger fields (will need to
> investigate more).  Once we fix this type of incompatibility references, we
> can change them.  Or we could remove these static final constants
> completely but it's less preferable since it touches many of the JDK files.
>  I would keep these static fields as a shortcut.
>
> 2. New convenient methods (isInfoLoggable, isWarningLoggable) to minimize
> the dependency to the constants
>
> It's not a problem to have the dependencies.  This is an issue this time
> since we were not aware of such dependency.  The isLoggable method is
> simple enough.
>
> 3. 3 methods with two versions (one with int and one with Level parameter)
>
> As I said, I'll file a bug to remove the 3 deprecated methods in jdk8. I'm
> certain I can do so but just take some time to synchronize the changes.
>
> 4. It's true that you can set a PlatformLogger with a custom level via
> PlatformLogger API.  But you can access a platform logger using
> java.util.logging API.
>
>    Logger.getLogger("sun.awt.**someLogger").setLevel(MyLevel.**
> CUSTOM_LEVEL);
>
> PlatformLogger.getLevel() has to return some thing.  Laurent suggests to
> remove (deprecate) PlatformLogger.getLevel() or level() method.  I have to
> understand how the FX code uses getLevel().  We have to keep it due to the
> regression and for testing.  For API perspective, having a method to find
> what level it's at is reasonable.  Since we now have a clean solution to
> deal with custom level, I don't see any issue keeping it.
>
> 5. JavaFX 8 is likely to switch to build with JDK8 in a few weeks (really
> soon).
>
> 6. Level.intValue() public or not
>
> It mirrors java.util.logging.Level but may be able to do without it.
>  Let's leave it public for now until FX converts to use the new code.  I
> can clean this up at the same time.
>
> Mandy
>
>
>
> On 4/3/13 9:52 PM, Mandy Chung wrote:
>
>> Peter, Laurent,
>>
>> History and details are described below.
>>
>> Webrev at:
>> http://cr.openjdk.java.net/~**mchung/jdk8/webrevs/8011380/**webrev.00/<http://cr.openjdk.java.net/%7Emchung/jdk8/webrevs/8011380/webrev.00/>
>>
>> I add back the getLevel(int), setLevel(int) and isLoggable(int) methods
>> but marked deprecated and also revert the static final variables to resolve
>> the regression. They can be removed when JavaFX transitions to use the
>> Level enums (I'll file one issue for FX to use PlatformLogger.Level and one
>> for core-libs to remove the deprecated methods).  The performance overhead
>> is likely small since it's direct mapping from int to the Level enum that
>> was used in one of your previous performance measurement.
>>
>> Laurent - you have a patch to add isLoggable calls in the awt/swing code.
>> Can you check if there is any noticeable performance difference?
>>
>> I also take the opportunity to reconsider what JavaLoggerProxy.getLevel()
>> should return when it's a custom Level. Use of logging is expected not to
>> cause any fatal error to the running application.  The previous patch
>> throwing IAE needs to be fixed.  I propose to return the first
>> Level.intValue() >= the custom level value since the level value is used to
>> evaluate if it's loggable.
>>
>> History and details:
>> JavaFX 8 has converted to use sun.util.logging.**PlatformLogger (
>> https://javafx-jira.kenai.**com/browse/RT-24458<https://javafx-jira.kenai.com/browse/RT-24458>).
>> I was involved in the early discussion but wasn't aware of the decision
>> made.  Thanks to Alan for catching this regression out before it's
>> integrated to jdk8.  jfxrt.jar is cobundled with jdk8 during the installer
>> build step.  My build doesn't build installer and thus we didn't see this
>> regression.
>>
>> I ran jdeps on jdk8/lib/ext/jfxrt.jar (windows-i586) that shows 112
>> references to PlatformLogger and on jdk7/lib/jfxrt.jar that shows no
>> reference to sun.util.logging.
>>
>> I have a method finder tool (planning to include it in jdk8) to search
>> for use of PlatformLogger.getLevel and it did find 2 references from
>> jdk8/lib/ext/jfxrt.jar.
>>
>> JavaFX 8 is going to upgrade to use JDK 8 for JavaFX build (
>> https://javafx-jira.kenai.**com/browse/RT-27794<https://javafx-jira.kenai.com/browse/RT-27794>)
>> soon (currently it's built with JDK 7).  As soon as JavaFX code are changed
>> to reference PlatformLogger.Level enum instead, we can remove the
>> deprecated methods and change the PlatformLogger constants.
>>
>> JavaFX 2.2.x doesn't use sun.util.logging and so this has no impact to
>> it.  In any case, JavaFX 2.2.x only runs either bundled with a
>> corresponding JDK 7u release, or as a standalone library for JDK 6 only.
>>
>> Thanks
>> Mandy
>>
>>
>