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
