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<http://download.java.net/jdk8/docs/api/java/util/function/Supplier.html>
<String> 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
>>
>>
>
>