Review request: 8011380: FX dependency on PlatformLogger broken (original) (raw)

Laurent Bourgès bourges.laurent at gmail.com
Thu Apr 4 08:15:41 UTC 2013

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

More information about the core-libs-dev mailing list