Review request: 8011380: FX dependency on PlatformLogger broken
bourges.laurent at gmail.com
Thu Apr 4 08:15:41 UTC 2013
here are my comments:
On 04/04/2013 06:52 AM, Mandy Chung wrote:
> Peter, Laurent,
> History and details are described below.
> Webrev at:
> 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
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
> 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
> 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:
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
More information about the core-libs-dev