Review request: 8011380: FX dependency on PlatformLogger broken

[Laurent Bourgès]

Mandy, Peter,

here are my comments:

On 04/04/2013 06:52 AM, 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.
>
> Look OK for me; the Level valueOf(int level) is back and is basically an
efficient switch case that performs well (performance overhead is minor). I
hope other projects will be built again soon to remove such workarrounds.

>
> I think that it is not strictly necessary to restore the PlatformLogger
> static final fields back to int type. They are compile-time constants and
> so are already "baked" into the classes compiled with older version of
> PlatformLogger. Am I right? The only problem I see if fields are kept as
> Level type is when JFX is compiled with JDK8 and then run on JDK7... But
> changing them temporarily back to int is a conservative approach and I
> support it.
>

I think you're right: static constants are copied into class bytecode;
maybe the old API (int level in method signatures) could be kept and marked
deprecated but the class will become quite big (double API: one with int
and one with Level) !!

>
>
> Laurent - you have a patch to add isLoggable calls in the awt/swing code.
> Can you check if there is any noticeable performance difference?
>
> The awt patch consists in adding missing isLoggable statements to avoid
object creation (string concatenations) and method calls
(Component.toString() ...).

Maybe I should also check that calls to log(msg, varargs) are using
isLoggable() tests to avoid Object[] creation due to varargs: what is your
opinion ?


>
> 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.
>
>
> That's a good compromise.
>

I think custom logger are ILLEGAL in the PlatformLogger API and must be
discarded. Maybe comments should explain such design decision and returning
an IllegalStateException is OK for me.


>
>
> 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.
>
> Personally I doubt getLevel() method is useful: why not use isLoggable()
instead !! maybe getLevel() should become @deprecated too.


>
> 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.
>
> Great, any idea about their roadmap ?
do you think other projects are also depending on PlatformLogger (JMX ...)
and may be impacted ?

>
> What do you think of deprecating the PlatformLogger constants altogether
> (regardless of their type). The code using them could be migrated to use
> PlatformLogger.Level enum members directly and if "
> PlatformLogger.Level.INFO" looks to verbose, static imports can help or
> there could be some more helper methods added to PlatformLogger that would
> minimize the need to access the constants like:
>
> isInfoLoggable();
> isWarningLoggable();
> ...
>

It starts to mimic log4j / slf4j / logback  syntax I love but I think it
will change so many files that I think it is a waste of time for now.

I vote for adding such useful shortcuts in the new API but not change other
methods.

Cheers,
Laurent