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

Fri, 10 Jul 2020 07:43:46 -0700 

That is a good question. I am not sure any more how we ended up with
the mixed usage but isLogging() seems appropriate to guard the call to log.

So maybe make them consistent for all cases that make sense.

If the end result of that is there is no other usage that calls debugFonts()
that might be something else to take a look if it is basically synonomous.

BTW I suppose you believe you have found all the cases in various packages
and platform folders and converted them ?
Or were there some cases you intentionally left alone ?
Not saying you missed any - just want to know what to expect when I go checking. 

-phil.

On 7/10/20, 12:34 AM, Baesken, Matthias wrote:
Hi Phil, okay get your point , thanks for clarification about avoiding the string concatenation if FontUtilities.isLogging() returns false .
But I guess the coding guarded by FontUtilities.debugFonts() for example :
src/java.desktop/share/classes/sun/awt/FontConfiguration.java-85- if (FontUtilities.debugFonts()) {
src/java.desktop/share/classes/sun/awt/FontConfiguration.java:86: FontUtilities.getLogger()
src/java.desktop/share/classes/sun/awt/FontConfiguration.java-87- .info("Creating standard Font Configuration"); 

would continue using  FontUtilities.debugFonts(),   correct ?

So this would change to :
src/java.desktop/share/classes/sun/awt/FontConfiguration.java-85- if (FontUtilities.debugFonts()) {
src/java.desktop/share/classes/sun/awt/FontConfiguration.java:86: FontUtilities.logInfo("Creating standard Font Configuration"); 

Best regards, Matthias

*From:*Philip Race <philip.r...@oracle.com>
*Sent:* Donnerstag, 9. Juli 2020 19:17
*To:* Baesken, Matthias <matthias.baes...@sap.com>
*Cc:* Peter Hull <peterhul...@gmail.com>; Jayathirth D v <jayathirth....@oracle.com>; Langer, Christoph <christoph.lan...@sap.com>; 2d-dev@openjdk.java.net *Subject:* Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to FontUtilities.java 


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 <http://cr.openjdk.java.net/%7Embaesken/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 <http://cr.openjdk.java.net/%7Embaesken/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
  
<http://cr.openjdk.java.net/%7Embaesken/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.r...@oracle.com> <mailto:philip.r...@oracle.com> 
    Sent: Donnerstag, 9. Juli 2020 17:04

    To: Peter Hull<peterhul...@gmail.com>  <mailto:peterhul...@gmail.com>

    Cc: Baesken, Matthias<matthias.baes...@sap.com>  <mailto:matthias.baes...@sap.com>; Jayathirth D 
v<jayathirth....@oracle.com>  <mailto:jayathirth....@oracle.com>; Langer, 
Christoph<christoph.lan...@sap.com>  <mailto:christoph.lan...@sap.com>;2d-dev@openjdk.java.net  
<mailto:2d-dev@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.baes...@sap.com>  
<mailto:matthias.baes...@sap.com>   wrote:

            Thank's  for the review !


            May I get a second  review ?


            Best regards, Matthias


            From: Jayathirth D v<jayathirth....@oracle.com>  
<mailto:jayathirth....@oracle.com>

            Sent: Donnerstag, 9. Juli 2020 07:21

            To: Baesken, Matthias<matthias.baes...@sap.com>  
<mailto:matthias.baes...@sap.com>

            Cc:2d-dev@openjdk.java.net  <mailto:2d-dev@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.baes...@sap.com>  
<mailto:matthias.baes...@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/  
<http://cr.openjdk.java.net/%7Embaesken/webrevs/8248802.0/>


            Thanks, Matthias

Reply via email to