Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Laurent Bourgès]

Mandy,

I believe the awt/net code started logging in 1.4 and 1.5 using j.u.logging
> that was really early taker before the performance overhead was considered.
>
> I filed a bug 8011557: improve the logging documentation to advice on
> performance consideration as we may want to mention this in the tutorial or
> other docs as well.  This should belong to java.util.logging instead of
> sun.util.logging.
>

That's a good idea to update both javadoc (JUL, PlatformLogger) and maybe
the java code convention related to OpenJDK code.

I insist on pointing this performance issue in the PlatformLogger class too
because it belongs to JDK and I expect it to be as fast as possible !

Moreover, it would be very great if openjdk's build can perform code
quality tests and report such incorrect JUL / PlatformLogger usages. Any
idea to do so ?


>
> Having a second thought, Supplier may not address the memory overhead
> concern you raise.  Worth considering any API improvement to address both
> the runtime and memory concern.
>

I looked at JDK8 Logger doc and Supplier may help (but maybe the underlying
implementation may be slow or create objects too):
"A set of methods alternatively take a "msgSupplier" instead of a "msg"
argument. These methods take a
Supplier
 function which is invoked to construct the desired log message
only when the message actually is to be logged based on the effective log
level thus eliminating unnecessary message construction. For example, if
the developer wants to log system health status for diagnosis, with the
String-accepting version, the code would look like:

   class DiagnosisMessages {
 static String systemHealthStatus() {
   // collect system health information
   ...
 }
   }
   ...
   logger.log(Level.FINER, DiagnosisMessages.systemHealthStatus());

*With the above code, the health status is collected unnecessarily even
when the log level FINER is disabled. With the Supplier-accepting version
as below, the status will only be collected when the log level FINER is
enabled. *

*   logger.log(Level.FINER, DiagnosisMessages::systemHealthStatus);*

"


> It would also be helpful if IDE and code analysis tool can hint the
> developers of such pattern.
>

Netbeans has a warning saying "string concatenation in log statements" but
it does not propose to fix it by adding proper isLoggable statement: maybe
we should submit a RFE to netbeans project ?
http://wiki.netbeans.org/Java_Hints#Logging

"*String concatenation in logger* It is not performance efficient to
concatenate strings in logger messages. It is better to use a template
message with placeholders that are replaced by concrete values only when
the message is really going to be logged. *Since NetBeans 6.9* "

Cheers,
Laurent


> Mandy
> P.S. I'll be pushing the changeset today.
>
>
> On 4/5/2013 9:02 AM, Laurent Bourgès wrote:
>
> Mandy,
>
> I agree it should be well known; but I fixed several cases in awt/net code
> where isLoggable() calls were missing.
>
> I think it is quite cheap to remind good practices in the PlatformLogger /
> jul Logger javadoc ...
>
> PS: maybe some quality control tools could check such missing tests (PMD
> can do it)...
>
>   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.
>>
>
> Very interesting feature, but I am still not aware of such JDK 8 features
> (lambda) ... it seems nice but maybe the syntax will be more complicated /
> verbose than our usual log statements:
> log.fine(""...)
>
> Laurent
>
>
>>
>> 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
>>
>>
>
>

Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Mandy Chung]

Laurent,

I believe the awt/net code started logging in 1.4 and 1.5 using 
j.u.logging that was really early taker before the performance overhead 
was considered.


I filed a bug 8011557: improve the logging documentation to advice on 
performance consideration as we may want to mention this in the tutorial 
or other docs as well.  This should belong to java.util.logging instead 
of sun.util.logging.


Having a second thought, Supplier may not address the memory overhead 
concern you raise.  Worth considering any API improvement to address 
both the runtime and memory concern.  It would also be helpful if IDE 
and code analysis tool can hint the developers of such pattern.


Mandy
P.S. I'll be pushing the changeset today.

On 4/5/2013 9:02 AM, Laurent Bourgès wrote:

Mandy,

I agree it should be well known; but I fixed several cases in awt/net 
code where isLoggable() calls were missing.


I think it is quite cheap to remind good practices in the 
PlatformLogger / jul Logger javadoc ...


PS: maybe some quality control tools could check such missing tests 
(PMD can do it)...


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.


Very interesting feature, but I am still not aware of such JDK 8 
features (lambda) ... it seems nice but maybe the syntax will be more 
complicated / verbose than our usual log statements:

log.fine(""...)

Laurent



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

Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Laurent Bourgès]

Mandy,

I agree it should be well known; but I fixed several cases in awt/net code
where isLoggable() calls were missing.

I think it is quite cheap to remind good practices in the PlatformLogger /
jul Logger javadoc ...

PS: maybe some quality control tools could check such missing tests (PMD
can do it)...

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.
>

Very interesting feature, but I am still not aware of such JDK 8 features
(lambda) ... it seems nice but maybe the syntax will be more complicated /
verbose than our usual log statements:
log.fine(""...)

Laurent


>
> 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
>
>

Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Mandy Chung]

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 >


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/


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/


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?

 

Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Alan Bateman]

On 04/04/2013 22:58, Mandy Chung wrote:

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/

I want to push the changeset tomorrow since we need this fix before 
the TL integration.
This looks fine. It's conservative in that it brings us back to the 
status quo and give FX time to change.


-Alan

Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Laurent Bourgès]

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 

> 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/
>
> 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/
>>
>> 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 invo

Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Mandy Chung]

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/

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/

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

Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Laurent Bourgès]

Well done: binary search to find the closest matching level !

However, I still do not see any concrete use case for custom Levels in such
closed API as PlatformLogger is: KISS, please !

I mean as PlatformLogger is only used (available) to JDK and related
projects, is there any RFE or will to use custom JUL levels here ?

Laurent

2013/4/4 Peter Levart 

> Hi Mandy, Laurent,
>
> What do you think of this variation (changed just PlatformLogger, other
> files exactly equal to your webrev):
>
> http://dl.dropbox.com/u/**101777488/jdk8-tl/**
> PlatformLogger/webrev.enumapi.**02/index.html
>
> Moved the nearest >= intValue search to PlatformLogger.Level.valueOf(**int)
> since it is used also by deprecated API. With this change the same logic is
> used for mapping from j.u.l.Level to PlatformLogger.Level in all cases.
>
> I wonder if PlatformLogger.Level.intValue(**) should be exposed as public
> API. Currently it is used by the test (which is not in the same package).
> But this could be circumvented with reflection. I only see the use of it if
> we also make the PlatformLogger.Level.valueOf(**int) public with
> anticipation of enabling PlatformLogger to use custom
> PlatformLogger.Level(s) like j.u.logging. This extension could be performed
> compatibly by transforming Level enum into a plain class like j.u.l.Level.
> But until then, I don't see the use of Level.intValue() as a public API.
> What do you think?
>
> Regards, Peter
>
>
> 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.
>>
>> 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
>>
>>
>

Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Peter Levart]

Hi Mandy, Laurent,

What do you think of this variation (changed just PlatformLogger, other 
files exactly equal to your webrev):


http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.enumapi.02/index.html

Moved the nearest >= intValue search to 
PlatformLogger.Level.valueOf(int) since it is used also by deprecated 
API. With this change the same logic is used for mapping from 
j.u.l.Level to PlatformLogger.Level in all cases.


I wonder if PlatformLogger.Level.intValue() should be exposed as public 
API. Currently it is used by the test (which is not in the same 
package). But this could be circumvented with reflection. I only see the 
use of it if we also make the PlatformLogger.Level.valueOf(int) public 
with anticipation of enabling PlatformLogger to use custom 
PlatformLogger.Level(s) like j.u.logging. This extension could be 
performed compatibly by transforming Level enum into a plain class like 
j.u.l.Level. But until then, I don't see the use of Level.intValue() as 
a public API. What do you think?


Regards, Peter

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.


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

Re: 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

Re: Review request: 8011380: FX dependency on PlatformLogger broken

[Peter Levart]

HI Mandy,

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.


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.




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.


That's a good compromise.



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.


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();
...


Regards, Peter



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

Review request: 8011380: FX dependency on PlatformLogger broken

[Mandy Chung]

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.


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