Laurent,
I believe this was mentioned somewhere in j.u.logging. A better
solution may be to take java.util.function.Supplier parameter that
constructs the log message lazily (see
http://download.java.net/jdk8/docs/api/java/util/logging/Logger.html#fine(java.util.function.Supplier).
I can file a RFE to investigate the use of Supplier as in j.u.l.Logger.
Thanks
Mandy
On 4/5/2013 1:55 AM, Laurent Bourgès wrote:
Mandy,
I would like to add few performance advices in the PlatformLogger Javadoc:
"
NOTE: For performance reasons, PlatformLogger usages should take care
of avoiding useless / wasted object creation and method calls related
to *disabled* log statements:
Always use isLoggable(level) wrapping logs at levels (FINEST, FINER,
FINE, CONFIG):
Bad practices:
- string concat:
log.fine("message" + value); // means
StringBuilder(message).append(String.valueOf(value)).toString(): 2
objects created and value.toString() called
- varags:
log.fine("message {0}", this); // create an Object[]
Best practices:
if (log.isLoggable(PlatformLogger.FINE) {
log.fine("message" + value);
}
if (log.isLoggable(PlatformLogger.FINE) {
log.fine("message {0}", this);
}
"
What is your opinion ?
Thanks for the given explanations and I hope that this patch will be
submitted soon to JDK8 repository.
Laurent
2013/4/4 Mandy Chung <mandy.ch...@oracle.com
<mailto:mandy.ch...@oracle.com>>
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/
<http://cr.openjdk.java.net/%7Emchung/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/
<http://cr.openjdk.java.net/%7Emchung/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
