Re: [OpenJDK 2D-Dev] Fwd: Re: RFR : 8248802: Add log helper methods to FontUtilities.java
Hi Daniel,
thanks for this good suggestion. Matthias change won't go that far now anyway
but if font logging was to be further refactored, this should definitely be
taken into account.
Best regards
Christoph
> -Original Message-
> From: Daniel Fuchs
> Sent: Dienstag, 14. Juli 2020 17:40
> To: Roger Riggs ; Phil Race
> ; Langer, Christoph ;
> 2d-dev@openjdk.java.net
> Subject: Re: Fwd: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper
> methods to FontUtilities.java
>
> Hi Christoph,
>
> Sorry - I'm not on 2d-dev - so please include me in cc: if you
> reply to this mail. Also my apologies if I don't have the full
> context of this discussion.
>
> > Unfortunately, PlatformLogger does not (yet?) offer public logging
> > methods taking suppliers.
>
> I would suggest using System.Logger directly instead.
> PlatformLogger delegates to System.Logger behind the scene,
> and System.Logger has APIs that take suppliers:
>
> https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/lang/Sy
> stem.Logger.html
>
> best regards
>
> -- daniel
>
> On 14/07/2020 16:03, Roger Riggs wrote:
> > Forwarded Message
> > Subject:Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper
> methods to
> > FontUtilities.java
> > Date: Tue, 14 Jul 2020 14:22:50 +
> > From: Langer, Christoph
> > To: Philip Race , Baesken, Matthias
> >
> > CC: Peter Hull , Jayathirth D v
> > , 2d-dev@openjdk.java.net
> > <2d-dev@openjdk.java.net>
> >
> >
> >
> > Hi,
> >
> > I guess it would make sense to offer logging methods that take a
> > supplier as input. That way we could pass String concatenations as
> > Lambdas that only evaluate when actually calling the logging.
> >
> > Unfortunately, PlatformLogger does not (yet?) offer public logging
> > methods taking suppliers. Those should, however, be easy to implement,
> > leveraging already existing signatures of the logging Bridge such as
> > here:
> >
> https://github.com/openjdk/jdk/blob/195c45a0e11207e15c277e7671b2a82b
> 8077c5fb/src/java.base/share/classes/sun/util/logging/PlatformLogger.java#
> L210
> >
> > Furthermore, initialization of logging in FontUtilities looks a bit
> > awkward. I think the if (debugFonts) in line 117 is unnecessary and the
> > code of that block could be added to the block before (of line 107: if
> > (debugLevel != null && !debugLevel.equals("false"))). And you could also
> > remove the following imports there (line 29ff):
> >
> > import java.io.BufferedReader;
> >
> > import java.io.File;
> >
> > import java.io.FileInputStream;
> >
> > import java.io.InputStreamReader;
> >
> > Best regards
> >
> > Christoph
> >
Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to FontUtilities.java
On 7/14/20, 8:43 AM, Langer, Christoph wrote: This should probably also be pushed to the client repo. That is not just "probably", it is a definite yes. -phil.
Re: [OpenJDK 2D-Dev] Fwd: Re: RFR : 8248802: Add log helper methods to FontUtilities.java
Hi Christoph,
Sorry - I'm not on 2d-dev - so please include me in cc: if you
reply to this mail. Also my apologies if I don't have the full
context of this discussion.
> Unfortunately, PlatformLogger does not (yet?) offer public logging
> methods taking suppliers.
I would suggest using System.Logger directly instead.
PlatformLogger delegates to System.Logger behind the scene,
and System.Logger has APIs that take suppliers:
https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/lang/System.Logger.html
best regards
-- daniel
On 14/07/2020 16:03, Roger Riggs wrote:
Forwarded Message
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to
FontUtilities.java
Date: Tue, 14 Jul 2020 14:22:50 +
From: Langer, Christoph
To: Philip Race , Baesken, Matthias
CC: Peter Hull , Jayathirth D v
, 2d-dev@openjdk.java.net
<2d-dev@openjdk.java.net>
Hi,
I guess it would make sense to offer logging methods that take a
supplier as input. That way we could pass String concatenations as
Lambdas that only evaluate when actually calling the logging.
Unfortunately, PlatformLogger does not (yet?) offer public logging
methods taking suppliers. Those should, however, be easy to implement,
leveraging already existing signatures of the logging Bridge such as
here:
https://github.com/openjdk/jdk/blob/195c45a0e11207e15c277e7671b2a82b8077c5fb/src/java.base/share/classes/sun/util/logging/PlatformLogger.java#L210
Furthermore, initialization of logging in FontUtilities looks a bit
awkward. I think the if (debugFonts) in line 117 is unnecessary and the
code of that block could be added to the block before (of line 107: if
(debugLevel != null && !debugLevel.equals("false"))). And you could also
remove the following imports there (line 29ff):
import java.io.BufferedReader;
import java.io.File;
import java.io.FileInputStream;
import java.io.InputStreamReader;
Best regards
Christoph
Re: [OpenJDK 2D-Dev] Fwd: Re: RFR : 8248802: Add log helper methods to FontUtilities.java
That would I think also take care of some "technical debt" which
is on the to-do list since I want to remove all unnecessary usage
of java.base internals.
-phil.
On 7/14/20, 8:39 AM, Daniel Fuchs wrote:
Hi Christoph,
Sorry - I'm not on 2d-dev - so please include me in cc: if you
reply to this mail. Also my apologies if I don't have the full
context of this discussion.
> Unfortunately, PlatformLogger does not (yet?) offer public logging
> methods taking suppliers.
I would suggest using System.Logger directly instead.
PlatformLogger delegates to System.Logger behind the scene,
and System.Logger has APIs that take suppliers:
https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/lang/System.Logger.html
best regards
-- daniel
On 14/07/2020 16:03, Roger Riggs wrote:
Forwarded Message
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper
methods to FontUtilities.java
Date: Tue, 14 Jul 2020 14:22:50 +
From: Langer, Christoph
To: Philip Race , Baesken, Matthias
CC: Peter Hull , Jayathirth D v
, 2d-dev@openjdk.java.net
<2d-dev@openjdk.java.net>
Hi,
I guess it would make sense to offer logging methods that take a
supplier as input. That way we could pass String concatenations as
Lambdas that only evaluate when actually calling the logging.
Unfortunately, PlatformLogger does not (yet?) offer public logging
methods taking suppliers. Those should, however, be easy to
implement, leveraging already existing signatures of the logging
Bridge such as here:
https://github.com/openjdk/jdk/blob/195c45a0e11207e15c277e7671b2a82b8077c5fb/src/java.base/share/classes/sun/util/logging/PlatformLogger.java#L210
Furthermore, initialization of logging in FontUtilities looks a bit
awkward. I think the if (debugFonts) in line 117 is unnecessary and
the code of that block could be added to the block before (of line
107: if (debugLevel != null && !debugLevel.equals("false"))). And you
could also remove the following imports there (line 29ff):
import java.io.BufferedReader;
import java.io.File;
import java.io.FileInputStream;
import java.io.InputStreamReader;
Best regards
Christoph
Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to FontUtilities.java
Hi Matthias,
this looks good to me. I agree that one could look at refactoring regarding
Suppliers and maybe also improving the isLogging checks in a subsequent change.
I found a few lines where spaces around the plus signs are missing. Maybe you
can correct this formatting before pushing:
CFontManager.java L99:, CMap.java L402, SunFontManager.java L565, L628
This should probably also be pushed to the client repo.
Thanks
Christoph
From: Baesken, Matthias
Sent: Dienstag, 14. Juli 2020 17:22
To: Philip Race
Cc: Peter Hull ; Jayathirth D v
; Langer, Christoph ;
2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to
FontUtilities.java
>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 ?
Hi Phil, I kept a few FontUtilities.getLogger - calls because they are a
bit special or they use later when logging a second parameter :
grep -nH -r "FontUtilities.getLogger(" *
java.desktop/share/classes/sun/font/SunFontManager.java:1634:
PlatformLogger logger = FontUtilities.getLogger();
java.desktop/share/classes/sun/font/SunFontManager.java:2898:&&
FontUtilities.getLogger().isLoggable(PlatformLogger.Level.INFO)) {
java.desktop/share/classes/sun/font/TrueTypeFont.java:366:
FontUtilities.getLogger().severe("While reading " + platName, e);
java.desktop/share/classes/sun/font/TrueTypeFont.java:386:
FontUtilities.getLogger().severe("While reading " + platName, e);
New webrev (that keeps the FontUtilities.isLogging() outside the new
FontUtilities.log* functions ) :
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/
I also adjusted
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/src/java.desktop/share/classes/sun/font/FontUtilities.java.frames.html
Following Christophs advice :
>Furthermore, initialization of logging in FontUtilities looks a bit awkward. I
>think the if (debugFonts) in line 117 is unnecessary and the code of that
>block could be added to the block before (of line 107: if (debugLevel != null
>&& >!debugLevel.equals("false"))). And you could also remove the following
>imports there (line 29ff):
>import java.io.BufferedReader;
>import java.io.File;
>import java.io.FileInputStream;
>import java.io.InputStreamReader;
However enhancing PlatformLogger with Supplier stuff is out of scope of this
change (maybe another one) .
Best regards, Matthias
From: Philip Race mailto:philip.r...@oracle.com>>
Sent: Freitag, 10. Juli 2020 16:45
To: Baesken, Matthias
mailto:matthias.baes...@sap.com>>
Cc: Peter Hull mailto:peterhul...@gmail.com>>;
Jayathirth D v mailto:jayathirth@oracle.com>>;
Langer, Christoph mailto:christoph.lan...@sap.com>>;
2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to
FontUtilities.java
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
Sent: Donnerstag, 9. Juli 2020 19:17
To: Baesken, Matthias
Cc: Peter Hull ;
Jayathirth D v ;
Langer, Christoph ;
2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] RFR :
Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to FontUtilities.java
>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 ?
Hi Phil, I kept a few FontUtilities.getLogger - calls because they are a
bit special or they use later when logging a second parameter :
grep -nH -r "FontUtilities.getLogger(" *
java.desktop/share/classes/sun/font/SunFontManager.java:1634:
PlatformLogger logger = FontUtilities.getLogger();
java.desktop/share/classes/sun/font/SunFontManager.java:2898:&&
FontUtilities.getLogger().isLoggable(PlatformLogger.Level.INFO)) {
java.desktop/share/classes/sun/font/TrueTypeFont.java:366:
FontUtilities.getLogger().severe("While reading " + platName, e);
java.desktop/share/classes/sun/font/TrueTypeFont.java:386:
FontUtilities.getLogger().severe("While reading " + platName, e);
New webrev (that keeps the FontUtilities.isLogging() outside the new
FontUtilities.log* functions ) :
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/
I also adjusted
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/src/java.desktop/share/classes/sun/font/FontUtilities.java.frames.html
Following Christophs advice :
>Furthermore, initialization of logging in FontUtilities looks a bit awkward. I
>think the if (debugFonts) in line 117 is unnecessary and the code of that
>block could be added to the block before (of line 107: if (debugLevel != null
>&& >!debugLevel.equals("false"))). And you could also remove the following
>imports there (line 29ff):
>import java.io.BufferedReader;
>import java.io.File;
>import java.io.FileInputStream;
>import java.io.InputStreamReader;
However enhancing PlatformLogger with Supplier stuff is out of scope of this
change (maybe another one) .
Best regards, Matthias
From: Philip Race
Sent: Freitag, 10. Juli 2020 16:45
To: Baesken, Matthias
Cc: Peter Hull ; Jayathirth D v
; Langer, Christoph ;
2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to
FontUtilities.java
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
Sent: Donnerstag, 9. Juli 2020 19:17
To: Baesken, Matthias
Cc: Peter Hull ;
Jayathirth D v ;
Langer, Christoph ;
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
-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
Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to FontUtilities.java
Hi,
I guess it would make sense to offer logging methods that take a supplier as
input. That way we could pass String concatenations as Lambdas that only
evaluate when actually calling the logging.
Unfortunately, PlatformLogger does not (yet?) offer public logging methods
taking suppliers. Those should, however, be easy to implement, leveraging
already existing signatures of the logging Bridge such as here:
https://github.com/openjdk/jdk/blob/195c45a0e11207e15c277e7671b2a82b8077c5fb/src/java.base/share/classes/sun/util/logging/PlatformLogger.java#L210
Furthermore, initialization of logging in FontUtilities looks a bit awkward. I
think the if (debugFonts) in line 117 is unnecessary and the code of that block
could be added to the block before (of line 107: if (debugLevel != null &&
!debugLevel.equals("false"))). And you could also remove the following imports
there (line 29ff):
import java.io.BufferedReader;
import java.io.File;
import java.io.FileInputStream;
import java.io.InputStreamReader;
Best regards
Christoph
From: Philip Race
Sent: Freitag, 10. Juli 2020 16:45
To: Baesken, Matthias
Cc: Peter Hull ; Jayathirth D v
; Langer, Christoph ;
2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to
FontUtilities.java
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
Sent: Donnerstag, 9. Juli 2020 19:17
To: Baesken, Matthias
Cc: Peter Hull ;
Jayathirth D v ;
Langer, Christoph ;
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
-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 FontUtilit
