Review request: 8011380: FX dependency on PlatformLogger broken

[Mandy Chung]

Laurent,

I believe this was mentioned somewhere in j.u.logging.  A better 
solution may be to take java.util.function.Supplier parameter that 
constructs the log message lazily (see 
http://download.java.net/jdk8/docs/api/java/util/logging/Logger.html#fine(java.util.function.Supplier). 
I can file a RFE to investigate the use of Supplier as in j.u.l.Logger.

Thanks
Mandy

On 4/5/2013 1:55 AM, Laurent Bourgès wrote:
> 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 
> <mailto: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). 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) 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
>
>
>