[OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to FontUtilities.java

[Philip Race]

There is no harm in repeating isLogging() inside logWarning() but
I don't think it is sufficient.

What we mean is that in some cases the code looks now like this :-

http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/src/java.desktop/share/classes/sun/font/FileFontStrike.java.udiff.html

-            if (FontUtilities.isLogging()) {
-                FontUtilities.getLogger().warning(
-                        "Failed to render glyph using GDI: code=" + glyphCode
+            FontUtilities.logWarning("Failed to render glyph using GDI: code=" + glyphCode
                                  + ", fontFamily=" + family + ", style=" + style
                                  + ", size=" + size);
-            }


So all that string concatenation always happens, even if we don't log.

The code before probably wasn't 100% consistent but if updating it
then I suggest to make it 100% consistent to always check isLogging()
before calling logWarning().

I suggest to do it even in cases like this
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/src/java.desktop/share/classes/sun/font/CMap.java.udiff.html

+                FontUtilities.logWarning("Cmap UVS subtable overflows buffer.");


where there is no concatenation work happening, just to establish a
consistent pattern.


-phil.

On 7/9/20, 8:07 AM, Baesken, Matthias wrote:
>> There always should be a call such as FontUtilities.isLogging() test
>> protecting doing unnecessary work.
> Hi,  in my patch I always  call  isLogging()  in the new methods in
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/src/java.desktop/share/classes/sun/font/FontUtilities.java.frames.html
>
> So are you talking about other places in the coding ?
>
> Best regards, Matthias
>
>
>
> -----Original Message-----
> From: Philip Race<philip.race at oracle.com>
> Sent: Donnerstag, 9. Juli 2020 17:04
> To: Peter Hull<peterhull90 at gmail.com>
> Cc: Baesken, Matthias<matthias.baesken at sap.com>; Jayathirth D v<JAYATHIRTH.D.V at oracle.com>; Langer, Christoph<christoph.langer at sap.com>; 2d-dev at openjdk.java.net
> Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to FontUtilities.java
>
> I agree.
>
> There always should be a call such as FontUtilities.isLogging() test
> protecting doing unnecessary work.
>
>
> -phil.
>
> On 7/9/20, 3:24 AM, Peter Hull wrote:
>> Probably not my place to comment, but, does it matter that it's doing
>> unnecessary work evaluating the argument to logWarning et al, in the
>> case where logging is not enabled? It only seems to be string
>> concatenation and maybe would be optimised out anyway, I don't know.
>> Peter
>>
>> On Thu, 9 Jul 2020 at 08:32, Baesken, Matthias<matthias.baesken at sap.com>   wrote:
>>> Thank's  for the review !
>>>
>>> May I get a second  review ?
>>>
>>>
>>>
>>>
>>>
>>> Best regards, Matthias
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> From: Jayathirth D v<JAYATHIRTH.D.V at ORACLE.COM>
>>> Sent: Donnerstag, 9. Juli 2020 07:21
>>> To: Baesken, Matthias<matthias.baesken at sap.com>
>>> Cc: 2d-dev at openjdk.java.net
>>> Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to FontUtilities.java
>>>
>>>
>>>
>>> Looks good to me.
>>>
>>>
>>>
>>> Thanks,
>>>
>>> Jay
>>>
>>>
>>>
>>> On 06-Jul-2020, at 12:43 PM, Baesken, Matthias<matthias.baesken at sap.com>   wrote:
>>>
>>>
>>>
>>> Hello, please review this small change to font related logging .
>>>
>>>
>>>
>>> We have a lot of font logging calls in java.desktop that look similar to this coding :
>>>
>>>           if (FontUtilities.isLogging()) {
>>>               FontUtilities.getLogger().info("Here comes my important info");
>>>           }
>>>
>>> This coding could be simplified by adding static log methods to FontUtilities.java
>>>
>>>
>>>
>>> public static void logWarning(String s);
>>>
>>> public static void logInfo(String s);
>>>
>>> public static void logSevere(String s);
>>>
>>>
>>>
>>>     doing the isLogging check + FontUtilities.getLogger(). ...
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> Bug/webrev :
>>>
>>>
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8248802
>>>
>>>
>>>
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/
>>>
>>>
>>>
>>>
>>>
>>> Thanks, Matthias
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20200709/59b14908/attachment-0001.htm>