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 <[email protected]>
Sent: Donnerstag, 9. Juli 2020 19:17
To: Baesken, Matthias <[email protected]>
Cc: Peter Hull <[email protected]>; Jayathirth D v
<[email protected]>; Langer, Christoph <[email protected]>;
[email protected]
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
- 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 <[email protected]><mailto:[email protected]>
Sent: Donnerstag, 9. Juli 2020 17:04
To: Peter Hull <[email protected]><mailto:[email protected]>
Cc: Baesken, Matthias
<[email protected]><mailto:[email protected]>; Jayathirth D v
<[email protected]><mailto:[email protected]>; Langer,
Christoph <[email protected]><mailto:[email protected]>;
[email protected]<mailto:[email protected]>
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<[email protected]><mailto:[email protected]> wrote:
Thank's for the review !
May I get a second review ?
Best regards, Matthias
From: Jayathirth D
v<[email protected]><mailto:[email protected]>
Sent: Donnerstag, 9. Juli 2020 07:21
To: Baesken, Matthias<[email protected]><mailto:[email protected]>
Cc: [email protected]<mailto:[email protected]>
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<[email protected]><mailto:[email protected]> 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
