Review request: 8011380: FX dependency on PlatformLogger broken

[Mandy Chung]

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/

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/
>
> 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
>