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

Philip Race philip.race at oracle.com
Fri Jul 10 14:44:30 UTC 2020


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.race at oracle.com>
> *Sent:* Donnerstag, 9. Juli 2020 19:17
> *To:* Baesken, Matthias <matthias.baesken at sap.com>
> *Cc:* Peter Hull <peterhull90 at gmail.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
>
>
> 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.race at oracle.com>  <mailto:philip.race at oracle.com>  
>
>     Sent: Donnerstag, 9. Juli 2020 17:04
>
>     To: Peter Hull<peterhull90 at gmail.com>  <mailto:peterhull90 at gmail.com>
>
>     Cc: Baesken, Matthias<matthias.baesken at sap.com>  <mailto:matthias.baesken at sap.com>; Jayathirth D v<JAYATHIRTH.D.V at oracle.com>  <mailto:JAYATHIRTH.D.V at oracle.com>; Langer, Christoph<christoph.langer at sap.com>  <mailto:christoph.langer at sap.com>;2d-dev at openjdk.java.net  <mailto: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>  <mailto: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>  <mailto:JAYATHIRTH.D.V at ORACLE.COM>
>
>             Sent: Donnerstag, 9. Juli 2020 07:21
>
>             To: Baesken, Matthias<matthias.baesken at sap.com>  <mailto:matthias.baesken at sap.com>
>
>             Cc:2d-dev at openjdk.java.net  <mailto: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>  <mailto: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/  <http://cr.openjdk.java.net/%7Embaesken/webrevs/8248802.0/>
>
>               
>
>               
>
>               
>
>               
>
>               
>
>             Thanks, Matthias
>
>               
>
>               
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/2d-dev/attachments/20200710/a37c01b4/attachment-0001.htm>


More information about the 2d-dev mailing list