8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Here is an improved patch tested on JDK7u13 and JDK8 internal build on my
machine linux x64:
http://jmmc.fr/~bourgesl/share/webrev-8010309/
FYI, I removed completely the MapInteger, Object levelObjects and used
two arrays to perform the PlatformLogger's level (int) to j.u.l.Level
mapping:
I decided to keep it simple as possible (no enum ...) and used a switch
case based on current level occurences:
510 /**
511 * Return the corresponding j.u.l.Level instance
512 * @param level PlatformLogger level as integer
513 * @return Object (j.u.l.Level instance) or null if no
matching level
514 */
515 private static Object getLevel(final int level) {
516 if (levelObjects == null) {
517 return null;
518 }
519 // higher occurences first (finest, fine, finer, info)
520 // based on isLoggable(level) calls (03/20/2013)
521 // in jdk project only (including generated sources)
522 switch (level) {
523 case FINEST : return levelObjects[IDX_FINEST];
// 116 + 2257 matches in generated files
524 case FINE: return levelObjects[IDX_FINE];// 270
525 case FINER : return levelObjects[IDX_FINER]; // 157
526 case INFO: return levelObjects[IDX_INFO];// 39
527 case WARNING : return levelObjects[IDX_WARNING]; // 12
528 case CONFIG : return levelObjects[IDX_CONFIG]; // 6
529 case SEVERE : return levelObjects[IDX_SEVERE]; // 1
530 case OFF : return levelObjects[IDX_OFF]; // 0
531 case ALL : return levelObjects[IDX_ALL]; // 0
532 default : return null;
533 }
534 }
I enhanced the PlatformLoggerTest class also and figured out that TLAB
optimized Integer allocations but I think the patch is still useful.
Can you have a look to the patch ?
Should I write a jtreg test (performance / GC issue) ?
Cheers,
Laurent
2013/3/20 Mandy Chung mandy.ch...@oracle.com
Hi Laurent,
Thank you for signing the OCA. Your contribution is very welcome. You
can submit a patch for this bug (see [1]) to Core libraries group which
owns logging. Jim Gish and I will sponsor it.
Thanks
Mandy
[1] http://openjdk.java.net/contribute/
On 3/20/2013 5:45 AM, Laurent Bourgès wrote:
Hi mandy,
Do you want me to propose an improved patch to remove the former Map and
fix the getLevel() method ? or you prefer fix on your own ?
Is it better to discuss the fix on the bug database (still not visible) ?
Laurent
2013/3/19 Mandy Chung mandy.ch...@oracle.com
Hi Laurent,
Thanks for the contribution. I agree that the map can be replaced with a
direct mapping from a int value to Level object and avoid the autoboxing
conversion.
I have filed a bug to track this and target this for JDK8:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id= 8010309
Thanks
Mandy
On 3/19/13 5:19 AM, Laurent Bourgès wrote:
Dear all,
I run recently netbeans profiler on my swing application
(Aspro2:http://www.jmmc.fr/aspro
) under linux x64 platform and figured out a
performance and waste issue related to PlatformLogger.
Actually, the JavaLogger implementation uses a MapInteger, Object
levelObjects to store mapping between PlatformLogger's levels (int) and JUL
Level instances.
However, the isLoggable(int level) method is highly used by awt project and
other JDK projects and it leads to many Integer allocations as autoboxing
converts the level as int to an Integer instance used by the Map.get() call.
/**
* JavaLogger forwards all the calls to its corresponding
* java.util.logging.Logger object.
*/
static class JavaLogger extends LoggerProxy {
private static final* MapInteger, Object* levelObjects = new
HashMap();
...
public boolean isLoggable(*int level*) {
return LoggingSupport.isLoggable(javaLogger, *
levelObjects.get(level)*);
}
}
I wrote a simple test to illustrate that performance / waste problem:
PlatformLoggerTest that simply performs 1 million DISABLED log statements:
if (log.isLoggable(PlatformLogger.FINEST)) {
log.finest(test PlatformLogger.FINEST);
}
As you can see in screenshots:
- 5 million Integer instances are allocated
- Integer.valueOf(int) is called 5 million times (autoboxing)
- HashMap.get() represents 30% of the test time
- patched PlatformLogger is 3x times faster
[jvm options: -Xms8m -Xmx8m -verbose:gc]
I suggest you to use an alternative way to map PlatformLogger's levels
(int) and JUL Level instances to fix that performance / memory issue: I
added the getLevel(int level) method that performs a switch case to return
the corresponding Level object (quick and dirty solution).
I advocate this is not a very clean
JAX-WS update coming soon
Just a heads-up that there is a JAX-WS update coming for jdk8. Miroslav Kos will be sending a webrev soon with the changes that update what we have in jdk8 from 2.2.7-b09 to 2.2.9-b13922. The patch file is likely be 100k lines so impossible to do a complete review. The tentative plan is to push this via jdk8/tl/jaxws and any help with the review would be appreciated. -Alan.
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Here is the test code that was scrubbed in my previous email.
As you can see in my benchmarks:
- TLAB has a major impact on Integer allocation overhead
- patched code is 2x faster (with TLAB enabled)
Test code:
package test;
import java.util.logging.Level;
import java.util.logging.Logger;
import sun.util.logging.LoggingSupport;
import sun.util.logging.PlatformLogger;
/**
* PlatformLogger patch Test (performance / memory overhead)
* @author bourgesl
*/
public class PlatformLoggerTest {
/** logger - enable java.util.logging to enable PlatformLogger using
JUL */
private final static Logger logger =
Logger.getLogger(PlatformLoggerTest.class.getName());
public static void main(String[] args) {
/*
* 1/ JVM options during tests:
* -Xms8m -Xmx8m -XX:-UseTLAB -XX:+PrintTLAB
*
* JDK7_13 results:
mars 21, 2013 11:15:07 AM test.PlatformLoggerTest main
INFO: PlatformLoggerTest: start on JVM1.7.0_13 [Java HotSpot(TM)
64-Bit Server VM 23.7-b01]
*
INFO: testPerf[10 iterations]: duration = 61.5364606
ms.
INFO: PlatformLoggerTest: starting 1 iterations ...
INFO: testPerf[1 iterations]: duration = 10485.07581 ms.
INFO: testPerf[1 iterations]: duration = 10639.329926 ms.
INFO: testPerf[1 iterations]: duration = 10903.235198 ms.
INFO: testPerf[1 iterations]: duration = 10728.399372 ms.
INFO: testPerf[1 iterations]: duration = 10643.329983 ms.
INFO: testPerf[1 iterations]: duration = 10720.43687 ms.
INFO: testPerf[1 iterations]: duration =
10864.37159599 ms.
INFO: testPerf[1 iterations]: duration = 10713.845459 ms.
INFO: testPerf[1 iterations]: duration = 10458.257711 ms.
INFO: testPerf[1 iterations]: duration = 10606.267606 ms.
*
* OpenJDK8 (+patch):
mars 21, 2013 11:19:03 AM test.PlatformLoggerTest main
Infos: PlatformLoggerTest: start on JVM1.8.0-internal [OpenJDK
64-Bit Server VM 25.0-b22]
*
Infos: testPerf[10 iterations]: duration = 24.862816 ms.
Infos: PlatformLoggerTest: starting 1 iterations ...
Infos: testPerf[1 iterations]: duration = 1043.957166 ms.
Infos: testPerf[1 iterations]: duration = 1013.602486 ms.
Infos: testPerf[1 iterations]: duration =
1018.279262999 ms.
Infos: testPerf[1 iterations]: duration = 1073.422125 ms.
Infos: testPerf[1 iterations]: duration = 1024.742149 ms.
Infos: testPerf[1 iterations]: duration =
1021.316633999 ms.
Infos: testPerf[1 iterations]: duration = 1014.771751 ms.
Infos: testPerf[1 iterations]: duration =
1017.614895999 ms.
Infos: testPerf[1 iterations]: duration =
1021.442538999 ms.
Infos: testPerf[1 iterations]: duration = 1020.200104 ms.
*
* 2/ JVM options during tests:
* -Xms8m -Xmx8m -XX:+UseTLAB
*
* JDK7_13 results:
* mars 21, 2013 12:58:37 PM test.PlatformLoggerTest main
INFO: PlatformLoggerTest: start on JVM1.7.0_13 [Java HotSpot(TM)
64-Bit Server VM 23.7-b01]
*
INFO: testPerf[10 iterations]: duration = 55.329637 ms.
INFO: PlatformLoggerTest: starting 1 iterations ...
INFO: testPerf[1 iterations]: duration = 2553.872667 ms.
INFO: testPerf[1 iterations]: duration = 2327.072791 ms.
INFO: testPerf[1 iterations]: duration = 2324.000677 ms.
INFO: testPerf[1 iterations]: duration =
2326.085992997 ms.
INFO: testPerf[1 iterations]: duration = 2325.34332 ms.
INFO: testPerf[1 iterations]: duration = 2322.579729 ms.
INFO: testPerf[1 iterations]: duration = 2322.170814 ms.
INFO: testPerf[1 iterations]: duration = 2324.055535 ms.
INFO: testPerf[1 iterations]: duration =
2432.678482997 ms.
INFO: testPerf[1 iterations]: duration = 2335.47692 ms.
*
* OpenJDK8 (+patch):
mars 21, 2013 1:00:30 PM test.PlatformLoggerTest main
Infos: PlatformLoggerTest: start on JVM1.8.0-internal [OpenJDK
64-Bit Server VM 25.0-b22]
*
Infos: testPerf[10 iterations]: duration = 22.0910898
ms.
Infos: PlatformLoggerTest: starting 1 iterations ...
Infos: testPerf[1 iterations]: duration = 1023.84232 ms.
Infos: testPerf[1 iterations]: duration =
1010.543790999 ms.
Infos: testPerf[1 iterations]: duration =
1004.667538999 ms.
Infos: testPerf[1 iterations]: duration =
1005.277866999 ms.
Infos: testPerf[1
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
On Thu, 21 Mar 2013 12:12:07 +0100, Laurent Bourgès wrote: [..] FYI, I removed completely the MapInteger, Object levelObjects and used two arrays to perform the PlatformLogger's level (int) to j.u.l.Level mapping: [..] FWIW I know of at least one (quite old) project that declares custom j.u.l. Levels and which would be broken by this change. I personally don't care (since I think both j.u.l. and custom log levels are wrong to begin with :), but it's something to keep in mind. Maybe a Map-based mapping could be used as fallback, while the array-indexed lookups take the heat off of the common case. Just a suggestion. cheers Holger
Re: Please review: surrogate fiddle
Am 21.03.2013 00:22, schrieb Martin Buchholz:
AbstractStringBuilder:
Instead
270 public int codePointBefore(int index) {
271 int i = index - 1;
272 if ((i 0) || (i = count)) {
273 throw new StringIndexOutOfBoundsException(index);
274 }
I suggest
270 public int codePointBefore(int index) {
271 if ((--index 0) || (index = count)) {
272 throw new StringIndexOutOfBoundsException(index);
273 }
, because if e.g. the initial value of index is 0, then -1 reflects the
out-of-bound condition,
but not the initial 0 to report in the StringIndexOutOfBoundsException.
(Hopefully the following redundant i 0 bounds check from value[i] becomes
elimited by JIT.)
Additional suggestions:
231 * @exception _String_IndexOutOfBoundsException ...
235 public int codePointAt(int index) {
259 * @exception _String_IndexOutOfBoundsException ...
263 public int codePointBefore(int index) {
For what it's worth to throw the more verbose StringIndexOutOfBoundsException, if it's not mentioned
in the spec?
Additional nit:
In line 231 there is an extra space before and behind IndexOutOfBoundsException (the behind one I
would like to see everywhere), but in most cases of @param and @exception, the 2nd space is missing.
Anyway @throws would be better than @exception
-Ulf
Re: Please review: surrogate fiddle
Am 21.03.2013 14:09, schrieb Ulf Zibis: Yes, difficult to decide, but keep in the back of your mind, it's called _String_IndexOutOfBoundsException, not _CodepointAt_IndexOutOfBoundsException Oops, I meant ... _CodePointBefore_IndexOutOfBoundsException
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Holger,
you are right: PlatformLogger does not care about custom Level
implementations because it relies on j.u.l.Logger to decide if the message
should be logged or not:
609 public boolean isLoggable(int level) {
610 return LoggingSupport.isLoggable(javaLogger, getLevel(level));
611 }
The former MapInteger, Object levelObjects I removed was only used to get
the j.u.l.Level instance corresponding to a PlatformLogger's level which
are defined as int constants.
FYI, the former map was filled with only j.u.l.Level instances
corresponding to all PlatformLogger's levels:
481 private static void getLevelObjects() {
482 // get all java.util.logging.Level objects
483 int[] levelArray = new int[] {OFF, SEVERE, WARNING,
INFO, CONFIG, FINE, FINER, FINEST, ALL};
484 for (int l : levelArray) {
485 Object level = LoggingSupport.parseLevel(getLevelName(l));
486 levelObjects.put(l, level);
487 }
488 }
I think I did not change the scope of the mapping i.e. PlatformLogger's
levels.
Cheers,
Laurent
2013/3/21 Laurent Bourgès bourges.laur...@gmail.com
Here is the test code that was scrubbed in my previous email.
As you can see in my benchmarks:
- TLAB has a major impact on Integer allocation overhead
- patched code is 2x faster (with TLAB enabled)
Test code:
package test;
import java.util.logging.Level;
import java.util.logging.Logger;
import sun.util.logging.LoggingSupport;
import sun.util.logging.PlatformLogger;
/**
* PlatformLogger patch Test (performance / memory overhead)
* @author bourgesl
*/
public class PlatformLoggerTest {
/** logger - enable java.util.logging to enable PlatformLogger using
JUL */
private final static Logger logger =
Logger.getLogger(PlatformLoggerTest.class.getName());
public static void main(String[] args) {
/*
* 1/ JVM options during tests:
* -Xms8m -Xmx8m -XX:-UseTLAB -XX:+PrintTLAB
*
* JDK7_13 results:
mars 21, 2013 11:15:07 AM test.PlatformLoggerTest main
INFO: PlatformLoggerTest: start on JVM1.7.0_13 [Java HotSpot(TM)
64-Bit Server VM 23.7-b01]
*
INFO: testPerf[10 iterations]: duration = 61.5364606
ms.
INFO: PlatformLoggerTest: starting 1 iterations ...
INFO: testPerf[1 iterations]: duration = 10485.07581 ms.
INFO: testPerf[1 iterations]: duration = 10639.329926 ms.
INFO: testPerf[1 iterations]: duration = 10903.235198 ms.
INFO: testPerf[1 iterations]: duration = 10728.399372 ms.
INFO: testPerf[1 iterations]: duration = 10643.329983 ms.
INFO: testPerf[1 iterations]: duration = 10720.43687 ms.
INFO: testPerf[1 iterations]: duration =
10864.37159599 ms.
INFO: testPerf[1 iterations]: duration = 10713.845459 ms.
INFO: testPerf[1 iterations]: duration = 10458.257711 ms.
INFO: testPerf[1 iterations]: duration = 10606.267606 ms.
*
* OpenJDK8 (+patch):
mars 21, 2013 11:19:03 AM test.PlatformLoggerTest main
Infos: PlatformLoggerTest: start on JVM1.8.0-internal [OpenJDK
64-Bit Server VM 25.0-b22]
*
Infos: testPerf[10 iterations]: duration = 24.862816 ms.
Infos: PlatformLoggerTest: starting 1 iterations ...
Infos: testPerf[1 iterations]: duration = 1043.957166 ms.
Infos: testPerf[1 iterations]: duration = 1013.602486 ms.
Infos: testPerf[1 iterations]: duration =
1018.279262999 ms.
Infos: testPerf[1 iterations]: duration = 1073.422125 ms.
Infos: testPerf[1 iterations]: duration = 1024.742149 ms.
Infos: testPerf[1 iterations]: duration =
1021.316633999 ms.
Infos: testPerf[1 iterations]: duration = 1014.771751 ms.
Infos: testPerf[1 iterations]: duration =
1017.614895999 ms.
Infos: testPerf[1 iterations]: duration =
1021.442538999 ms.
Infos: testPerf[1 iterations]: duration = 1020.200104 ms.
*
* 2/ JVM options during tests:
* -Xms8m -Xmx8m -XX:+UseTLAB
*
* JDK7_13 results:
* mars 21, 2013 12:58:37 PM test.PlatformLoggerTest main
INFO: PlatformLoggerTest: start on JVM1.7.0_13 [Java HotSpot(TM)
64-Bit Server VM 23.7-b01]
*
INFO: testPerf[10 iterations]: duration = 55.329637 ms.
INFO: PlatformLoggerTest: starting 1 iterations ...
INFO: testPerf[1 iterations]: duration = 2553.872667 ms.
INFO: testPerf[1 iterations]: duration = 2327.072791 ms.
INFO: testPerf[1 iterations]: duration =
Review request 8010416 Addition of Driver.deregisterDriver
Need a reviewer for 8010416, addition of Driver.deregisterDriver. The webrev can be found at http://cr.openjdk.java.net/~lancea/8010416/webrev.00/. I will be submitting the ccc request later today but want to do this in parallel Best Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
hg: jdk8/tl/jdk: 8009869: Need to modify java.security property package.access to include nashorn packages
Changeset: 9ee1aff76498 Author:sundar Date: 2013-03-21 19:19 +0530 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/9ee1aff76498 8009869: Need to modify java.security property package.access to include nashorn packages Reviewed-by: ahgross, jlaskey, lagergren ! src/share/lib/security/java.security-linux ! src/share/lib/security/java.security-macosx ! src/share/lib/security/java.security-solaris ! src/share/lib/security/java.security-windows
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
On 03/21/2013 12:12 PM, Laurent Bourgès wrote:
Here is an improved patch tested on JDK7u13 and JDK8 internal build on my
machine linux x64:
http://jmmc.fr/~bourgesl/share/webrev-8010309/
FYI, I removed completely the MapInteger, Object levelObjects and used
two arrays to perform the PlatformLogger's level (int) to j.u.l.Level
mapping:
I decided to keep it simple as possible (no enum ...) and used a switch
case based on current level occurences:
Hi Laurent,
In my experience enums are just the right and most compact tool for
coding such constant associations. Here's a quick try (ripping off your
optimized switch ;-):
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.01/index.html
...it adds 12 LOC to the original PlatformLogger and is 43 LOC less tha
your patch. In addition:
- only one switch instead of two (to maintain)
- no parallel IDX_ constants
What do you think?
Regards, Peter
510 /**
511 * Return the corresponding j.u.l.Level instance
512 * @param level PlatformLogger level as integer
513 * @return Object (j.u.l.Level instance) or null if no
matching level
514 */
515 private static Object getLevel(final int level) {
516 if (levelObjects == null) {
517 return null;
518 }
519 // higher occurences first (finest, fine, finer, info)
520 // based on isLoggable(level) calls (03/20/2013)
521 // in jdk project only (including generated sources)
522 switch (level) {
523 case FINEST : return levelObjects[IDX_FINEST];
// 116 + 2257 matches in generated files
524 case FINE: return levelObjects[IDX_FINE];// 270
525 case FINER : return levelObjects[IDX_FINER]; // 157
526 case INFO: return levelObjects[IDX_INFO];// 39
527 case WARNING : return levelObjects[IDX_WARNING]; // 12
528 case CONFIG : return levelObjects[IDX_CONFIG]; // 6
529 case SEVERE : return levelObjects[IDX_SEVERE]; // 1
530 case OFF : return levelObjects[IDX_OFF]; // 0
531 case ALL : return levelObjects[IDX_ALL]; // 0
532 default : return null;
533 }
534 }
I enhanced the PlatformLoggerTest class also and figured out that TLAB
optimized Integer allocations but I think the patch is still useful.
Can you have a look to the patch ?
Should I write a jtreg test (performance / GC issue) ?
Cheers,
Laurent
2013/3/20 Mandy Chung mandy.ch...@oracle.com
Hi Laurent,
Thank you for signing the OCA. Your contribution is very welcome. You
can submit a patch for this bug (see [1]) to Core libraries group which
owns logging. Jim Gish and I will sponsor it.
Thanks
Mandy
[1] http://openjdk.java.net/contribute/
On 3/20/2013 5:45 AM, Laurent Bourgès wrote:
Hi mandy,
Do you want me to propose an improved patch to remove the former Map and
fix the getLevel() method ? or you prefer fix on your own ?
Is it better to discuss the fix on the bug database (still not visible) ?
Laurent
2013/3/19 Mandy Chung mandy.ch...@oracle.com
Hi Laurent,
Thanks for the contribution. I agree that the map can be replaced with a
direct mapping from a int value to Level object and avoid the autoboxing
conversion.
I have filed a bug to track this and target this for JDK8:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id= 8010309
Thanks
Mandy
On 3/19/13 5:19 AM, Laurent Bourgès wrote:
Dear all,
I run recently netbeans profiler on my swing application
(Aspro2:http://www.jmmc.fr/aspro
) under linux x64 platform and figured out a
performance and waste issue related to PlatformLogger.
Actually, the JavaLogger implementation uses a MapInteger, Object
levelObjects to store mapping between PlatformLogger's levels (int) and JUL
Level instances.
However, the isLoggable(int level) method is highly used by awt project and
other JDK projects and it leads to many Integer allocations as autoboxing
converts the level as int to an Integer instance used by the Map.get() call.
/**
* JavaLogger forwards all the calls to its corresponding
* java.util.logging.Logger object.
*/
static class JavaLogger extends LoggerProxy {
private static final* MapInteger, Object* levelObjects = new
HashMap();
...
public boolean isLoggable(*int level*) {
return LoggingSupport.isLoggable(javaLogger, *
levelObjects.get(level)*);
}
}
I wrote a simple test to illustrate that performance / waste problem:
PlatformLoggerTest that simply performs 1 million DISABLED log statements:
if (log.isLoggable(PlatformLogger.FINEST)) {
log.finest(test PlatformLogger.FINEST);
}
As you can see in screenshots:
- 5 million Integer instances are allocated
-
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Peter,
your solution looks better; I wanted my patch to be simple, efficient and
only modify the JavaLogger class (localized changes).
In your patch, I have doubts related to lazy and conditional initialization
in JavaLogger (static initialization):
if (LoggingSupport.isAvailable()) {
// initialize ...
}
Does somebody have the knowledge about LoggingSupport.isAvailable()
and the lazy PlatformLogger initialization (JavaLogger are only used
when j.u.l is initialized) ?
What's happening if LoggingSupport.isAvailable() returns false in your patch ?
- LevelEnum instances are incorrectly initialized: object field is null !
- PlatformLogger is then broken ... as Level object are required by j.u.l calls
To fix both problems, moving the LevelEnum into JavaLogger should help
and check nulls on LevelEnum.object field.
Thanks for your feedback,
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
On 03/21/2013 12:12 PM, Laurent Bourgès wrote:
Here is an improved patch tested on JDK7u13 and JDK8 internal build on my
machine linux x64:
http://jmmc.fr/~bourgesl/**share/webrev-8010309/http://jmmc.fr/%7Ebourgesl/share/webrev-8010309/
FYI, I removed completely the MapInteger, Object levelObjects and used
two arrays to perform the PlatformLogger's level (int) to j.u.l.Level
mapping:
I decided to keep it simple as possible (no enum ...) and used a switch
case based on current level occurences:
Hi Laurent,
In my experience enums are just the right and most compact tool for coding
such constant associations. Here's a quick try (ripping off your optimized
switch ;-):
http://dl.dropbox.com/u/**101777488/jdk8-tl/**PlatformLogger/webrev.01/**
index.htmlhttp://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.01/index.html
...it adds 12 LOC to the original PlatformLogger and is 43 LOC less tha
your patch. In addition:
- only one switch instead of two (to maintain)
- no parallel IDX_ constants
What do you think?
Regards, Peter
510 /**
511 * Return the corresponding j.u.l.Level instance
512 * @param level PlatformLogger level as integer
513 * @return Object (j.u.l.Level instance) or null if no
matching level
514 */
515 private static Object getLevel(final int level) {
516 if (levelObjects == null) {
517 return null;
518 }
519 // higher occurences first (finest, fine, finer, info)
520 // based on isLoggable(level) calls (03/20/2013)
521 // in jdk project only (including generated sources)
522 switch (level) {
523 case FINEST : return levelObjects[IDX_FINEST];
// 116 + 2257 matches in generated files
524 case FINE: return levelObjects[IDX_FINE];//
270
525 case FINER : return levelObjects[IDX_FINER]; //
157
526 case INFO: return levelObjects[IDX_INFO];//
39
527 case WARNING : return levelObjects[IDX_WARNING]; //
12
528 case CONFIG : return levelObjects[IDX_CONFIG]; //
6
529 case SEVERE : return levelObjects[IDX_SEVERE]; //
1
530 case OFF : return levelObjects[IDX_OFF]; //
0
531 case ALL : return levelObjects[IDX_ALL]; //
0
532 default : return null;
533 }
534 }
I enhanced the PlatformLoggerTest class also and figured out that TLAB
optimized Integer allocations but I think the patch is still useful.
Can you have a look to the patch ?
Should I write a jtreg test (performance / GC issue) ?
Cheers,
Laurent
2013/3/20 Mandy Chung mandy.ch...@oracle.com
Hi Laurent,
Thank you for signing the OCA. Your contribution is very welcome. You
can submit a patch for this bug (see [1]) to Core libraries group which
owns logging. Jim Gish and I will sponsor it.
Thanks
Mandy
[1]
http://openjdk.java.net/**contribute/http://openjdk.java.net/contribute/
On 3/20/2013 5:45 AM, Laurent Bourgès wrote:
Hi mandy,
Do you want me to propose an improved patch to remove the former Map and
fix the getLevel() method ? or you prefer fix on your own ?
Is it better to discuss the fix on the bug database (still not visible) ?
Laurent
2013/3/19 Mandy Chung mandy.ch...@oracle.com
Hi Laurent,
Thanks for the contribution. I agree that the map can be replaced with
a
direct mapping from a int value to Level object and avoid the autoboxing
conversion.
I have filed a bug to track this and target this for JDK8:
http://bugs.sun.com/**bugdatabase/view_bug.do?bug_**id=http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8010309
Thanks
Mandy
On 3/19/13 5:19 AM, Laurent Bourgès wrote:
Dear all,
I run recently netbeans profiler on my swing application (Aspro2:
http://www.jmmc.fr/**aspro http://www.jmmc.fr/aspro
) under
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
On 03/21/2013 03:30 PM, Laurent Bourgès wrote:
Peter,
your solution looks better; I wanted my patch to be simple, efficient
and only modify the JavaLogger class (localized changes).
In your patch, I have doubts related to lazy and conditional
initialization in JavaLogger (static initialization):
if (LoggingSupport.isAvailable()) {
// initialize ...
}
In original code, if LoggingSupport.isAvailable() returned false,
levelObjects map remained empty and consequently null was used as the
level object passed to LoggingSupport methods. In LevelEnum I try to
keep this logic. When LevelEnum is first used, it's constants are
initialized and level objects with them. If
LoggingSupport.isAvailable() returns false, level objects are
initialized to null.
I just noticed there's a bug in initialization of the LevelEnum.UNKNOWN
member constant. It should not try to parse level object. Here's an update:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.02/index.html
But your concern might be correct. In my code LevelEnum is also used
from the LoggerProxy.format() method (in addition to all the places in
JavaLogger) to obtain the level name for formatting. If this method is
called the first time while LoggingSupport.isAvailable() returns false
and that happens before JavaLogger uses LevelEnum for the first time
(and at that time LoggingSupport.isAvailable() would return true), then
level objects will not be initialized correctly.
Lazy initialization of level objects might help (for the price of added
complexity)...
Regards, Peter
Does somebody have the knowledge about LoggingSupport.isAvailable()
and the lazy PlatformLogger initialization (JavaLogger are only used when j.u.l
is initialized) ?
What's happening if LoggingSupport.isAvailable() returns false in your patch ?
- LevelEnum instances are incorrectly initialized:object field is null !
- PlatformLogger is then broken ... as Level object are required by j.u.l calls
To fix both problems, moving theLevelEnum into JavaLogger should help and
check nulls onLevelEnum.object field.
Thanks for your feedback,
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com
On 03/21/2013 12:12 PM, Laurent Bourgès wrote:
Here is an improved patch tested on JDK7u13 and JDK8 internal
build on my
machine linux x64:
http://jmmc.fr/~bourgesl/share/webrev-8010309/
http://jmmc.fr/%7Ebourgesl/share/webrev-8010309/
FYI, I removed completely the MapInteger, Object
levelObjects and used
two arrays to perform the PlatformLogger's level (int) to
j.u.l.Level
mapping:
I decided to keep it simple as possible (no enum ...) and used
a switch
case based on current level occurences:
Hi Laurent,
In my experience enums are just the right and most compact tool
for coding such constant associations. Here's a quick try (ripping
off your optimized switch ;-):
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.01/index.html
...it adds 12 LOC to the original PlatformLogger and is 43 LOC
less tha your patch. In addition:
- only one switch instead of two (to maintain)
- no parallel IDX_ constants
What do you think?
Regards, Peter
510 /**
511 * Return the corresponding j.u.l.Level instance
512 * @param level PlatformLogger level as integer
513 * @return Object (j.u.l.Level instance) or null
if no
matching level
514 */
515 private static Object getLevel(final int level) {
516 if (levelObjects == null) {
517 return null;
518 }
519 // higher occurences first (finest, fine,
finer, info)
520 // based on isLoggable(level) calls (03/20/2013)
521 // in jdk project only (including generated
sources)
522 switch (level) {
523 case FINEST : return
levelObjects[IDX_FINEST];
// 116 + 2257 matches in generated files
524 case FINE: return
levelObjects[IDX_FINE];// 270
525 case FINER : return
levelObjects[IDX_FINER]; // 157
526 case INFO: return
levelObjects[IDX_INFO];// 39
527 case WARNING : return
levelObjects[IDX_WARNING]; // 12
528 case CONFIG : return
levelObjects[IDX_CONFIG]; // 6
529 case SEVERE : return
levelObjects[IDX_SEVERE]; // 1
530 case OFF : return
levelObjects[IDX_OFF]; // 0
531 case ALL : return
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Laurent, Peter,
I haven't looked at the patch yet. One thing worths mentioning is that
PlatformLogger was added for the platform code to use so as to avoid the
initialization of java.util.logging since logging is not turned on by
default and that to reduce the startup overhead. In addition, it also
enables the elimination of the core classes dependency from
java.util.logging for modularization effort. Therefore the
PlatformLogger only lazily looks up the Level object when
java.util.logging is present and also has been initialized by
application code.
Mandy
On 3/21/2013 7:45 AM, Peter Levart wrote:
On 03/21/2013 03:30 PM, Laurent Bourgès wrote:
Peter,
your solution looks better; I wanted my patch to be simple, efficient
and only modify the JavaLogger class (localized changes).
In your patch, I have doubts related to lazy and conditional
initialization in JavaLogger (static initialization):
if (LoggingSupport.isAvailable()) {
// initialize ...
}
In original code, if LoggingSupport.isAvailable() returned false,
levelObjects map remained empty and consequently null was used as the
level object passed to LoggingSupport methods. In LevelEnum I try to
keep this logic. When LevelEnum is first used, it's constants are
initialized and level objects with them. If
LoggingSupport.isAvailable() returns false, level objects are
initialized to null.
I just noticed there's a bug in initialization of the
LevelEnum.UNKNOWN member constant. It should not try to parse level
object. Here's an update:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.02/index.html
But your concern might be correct. In my code LevelEnum is also used
from the LoggerProxy.format() method (in addition to all the places in
JavaLogger) to obtain the level name for formatting. If this method is
called the first time while LoggingSupport.isAvailable() returns false
and that happens before JavaLogger uses LevelEnum for the first time
(and at that time LoggingSupport.isAvailable() would return true),
then level objects will not be initialized correctly.
Lazy initialization of level objects might help (for the price of
added complexity)...
Regards, Peter
Does somebody have the knowledge about LoggingSupport.isAvailable()
and the lazy PlatformLogger initialization (JavaLogger are only used when j.u.l
is initialized) ?
What's happening if LoggingSupport.isAvailable() returns false in your patch ?
- LevelEnum instances are incorrectly initialized:object field is null !
- PlatformLogger is then broken ... as Level object are required by j.u.l calls
To fix both problems, moving theLevelEnum into JavaLogger should help and
check nulls onLevelEnum.object field.
Thanks for your feedback,
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com
On 03/21/2013 12:12 PM, Laurent Bourgès wrote:
Here is an improved patch tested on JDK7u13 and JDK8 internal
build on my
machine linux x64:
http://jmmc.fr/~bourgesl/share/webrev-8010309/
http://jmmc.fr/%7Ebourgesl/share/webrev-8010309/
FYI, I removed completely the MapInteger, Object
levelObjects and used
two arrays to perform the PlatformLogger's level (int) to
j.u.l.Level
mapping:
I decided to keep it simple as possible (no enum ...) and
used a switch
case based on current level occurences:
Hi Laurent,
In my experience enums are just the right and most compact tool
for coding such constant associations. Here's a quick try
(ripping off your optimized switch ;-):
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.01/index.html
...it adds 12 LOC to the original PlatformLogger and is 43 LOC
less tha your patch. In addition:
- only one switch instead of two (to maintain)
- no parallel IDX_ constants
What do you think?
Regards, Peter
510 /**
511 * Return the corresponding j.u.l.Level instance
512 * @param level PlatformLogger level as integer
513 * @return Object (j.u.l.Level instance) or
null if no
matching level
514 */
515 private static Object getLevel(final int level) {
516 if (levelObjects == null) {
517 return null;
518 }
519 // higher occurences first (finest, fine,
finer, info)
520 // based on isLoggable(level) calls
(03/20/2013)
521 // in jdk project only (including generated
sources)
522 switch (level) {
523 case FINEST : return
levelObjects[IDX_FINEST];
// 116 + 2257 matches in generated files
524 case FINE: return
levelObjects[IDX_FINE];//
Re: zip64 compatibility problems
Martin, 8007905 has been approved. On 2/10/13 10:30 PM, Xueming Shen wrote: 8007905. But you will have to hold the push. I will send it through the CCC. -Sherman On 2/10/2013 7:59 PM, Martin Buchholz wrote: On Sat, Feb 9, 2013 at 1:50 AM, Peter Levartpeter.lev...@gmail.com wrote: Also, this does not disable ZIP64 - it only disables it when it is not needed (most other zip implementations can still read the output) inhibit seems better than disable. disfavor ? I went for inhibit. There's now a shiny new test case . Same Bat Place: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/useZip64For64kEntries/ I am now happy with this change and would like to submit. As always, I need a bug id.
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Thanks Mandy for the clarifications !
Peter, I propose to:
- move the LevelEnum into the JavaLogger class in order to initialize it as
later as possible i.e. after j.u.l calls redirectPlatformLoggers()
- only use it in JavaLogger class (private enum) so revert changes to
PlatformLogger and LoggerProxy classes
- add few comments in the code to explain lazy initialization (see mandy's
answer)
Finally, could you keep my comment before the switch case (high occurences
first) ?
+static LevelEnum forValue(int levelValue) {
+// higher occurences first (finest, fine, finer, info)
+// based on isLoggable(level) calls (03/20/2013)
+// in jdk project only (including generated sources)
+switch (levelValue) {
+case PlatformLogger.FINEST: return FINEST; // 116 +
2257 matches in generated files
+case PlatformLogger.FINE:return FINE;// 270
+case PlatformLogger.FINER: return FINER; // 157
+case PlatformLogger.INFO:return INFO;// 39
+case PlatformLogger.WARNING: return WARNING; // 12
+case PlatformLogger.CONFIG: return CONFIG; // 6
+case PlatformLogger.SEVERE: return SEVERE; // 1
+case PlatformLogger.OFF: return OFF; // 0
+case PlatformLogger.ALL: return ALL; // 0
+default: return UNKNOWN;
+}
+}
cheers,
Laurent
2013/3/21 Mandy Chung mandy.ch...@oracle.com
Laurent, Peter,
I haven't looked at the patch yet. One thing worths mentioning is that
PlatformLogger was added for the platform code to use so as to avoid the
initialization of java.util.logging since logging is not turned on by
default and that to reduce the startup overhead. In addition, it also
enables the elimination of the core classes dependency from
java.util.logging for modularization effort. Therefore the PlatformLogger
only lazily looks up the Level object when java.util.logging is present and
also has been initialized by application code.
Mandy
On 3/21/2013 7:45 AM, Peter Levart wrote:
On 03/21/2013 03:30 PM, Laurent Bourgès wrote:
Peter,
your solution looks better; I wanted my patch to be simple, efficient and
only modify the JavaLogger class (localized changes).
In your patch, I have doubts related to lazy and conditional
initialization in JavaLogger (static initialization):
if (LoggingSupport.isAvailable()) {
// initialize ...
}
In original code, if LoggingSupport.isAvailable() returned false, levelObjects
map remained empty and consequently null was used as the level object
passed to LoggingSupport methods. In LevelEnum I try to keep this logic.
When LevelEnum is first used, it's constants are initialized and level
objects with them. If
LoggingSupport.isAvailable() returns false, level objects are initialized
to null.
I just noticed there's a bug in initialization of the LevelEnum.UNKNOWN
member constant. It should not try to parse level object. Here's an update:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.02/index.html
But your concern might be correct. In my code LevelEnum is also used from
the LoggerProxy.format() method (in addition to all the places in
JavaLogger) to obtain the level name for formatting. If this method is
called the first time while LoggingSupport.isAvailable() returns false
and that happens before JavaLogger uses LevelEnum for the first time (and
at that time LoggingSupport.isAvailable() would return true), then level
objects will not be initialized correctly.
Lazy initialization of level objects might help (for the price of added
complexity)...
Regards, Peter
Does somebody have the knowledge about LoggingSupport.isAvailable()
and the lazy PlatformLogger initialization (JavaLogger are only used when
j.u.l is initialized) ?
What's happening if LoggingSupport.isAvailable() returns false in your patch
? - LevelEnum instances are incorrectly initialized: object field is null !
- PlatformLogger is then broken ... as Level object are required by j.u.l
calls
To fix both problems, moving the LevelEnum into JavaLogger should help and
check nulls on LevelEnum.object field.
Thanks for your feedback,Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
On 03/21/2013 12:12 PM, Laurent Bourgès wrote:
Here is an improved patch tested on JDK7u13 and JDK8 internal build on my
machine linux x64:
http://jmmc.fr/~bourgesl/share/webrev-8010309/
FYI, I removed completely the MapInteger, Object levelObjects and used
two arrays to perform the PlatformLogger's level (int) to j.u.l.Level
mapping:
I decided to keep it simple as possible (no enum ...) and used a switch
case based on current level occurences:
Hi Laurent,
In my experience enums are just the right and most compact tool for
coding such constant associations. Here's a quick
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Hi Laurent,
I checked and LoggingSupport.isAvailable() can only return false in two
situations:
- no java.util.logging.LoggingProxyImpl class is found (but that is a
permanent condition and almost all LoggingSupport methods are
non-operational in that case)
- there is a circularity of class initialization present and a dependent
class tries to use LoggingSupport before it's static final field is set.
That is only possible if java.util.logging.LoggingProxyImpl
initialization caused that circularity. LoggingProxyImpl is only
dependent on sun.util.logging.LoggingProxy interface which has no static
initializers and LoggingProxyImpl has a simple static initializer
setting static final INSTANCE field to a singleton instance and it has
an empty private constructor. Therefore no class initialization
circularity is possible in this situation (I think).
Regards, Peter
On 03/21/2013 03:45 PM, Peter Levart wrote:
On 03/21/2013 03:30 PM, Laurent Bourgès wrote:
Peter,
your solution looks better; I wanted my patch to be simple, efficient
and only modify the JavaLogger class (localized changes).
In your patch, I have doubts related to lazy and conditional
initialization in JavaLogger (static initialization):
if (LoggingSupport.isAvailable()) {
// initialize ...
}
In original code, if LoggingSupport.isAvailable() returned false,
levelObjects map remained empty and consequently null was used as the
level object passed to LoggingSupport methods. In LevelEnum I try to
keep this logic. When LevelEnum is first used, it's constants are
initialized and level objects with them. If
LoggingSupport.isAvailable() returns false, level objects are
initialized to null.
I just noticed there's a bug in initialization of the
LevelEnum.UNKNOWN member constant. It should not try to parse level
object. Here's an update:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.02/index.html
But your concern might be correct. In my code LevelEnum is also used
from the LoggerProxy.format() method (in addition to all the places in
JavaLogger) to obtain the level name for formatting. If this method is
called the first time while LoggingSupport.isAvailable() returns false
and that happens before JavaLogger uses LevelEnum for the first time
(and at that time LoggingSupport.isAvailable() would return true),
then level objects will not be initialized correctly.
Lazy initialization of level objects might help (for the price of
added complexity)...
Regards, Peter
Does somebody have the knowledge about LoggingSupport.isAvailable()
and the lazy PlatformLogger initialization (JavaLogger are only used when j.u.l
is initialized) ?
What's happening if LoggingSupport.isAvailable() returns false in your patch ?
- LevelEnum instances are incorrectly initialized:object field is null !
- PlatformLogger is then broken ... as Level object are required by j.u.l calls
To fix both problems, moving theLevelEnum into JavaLogger should help and
check nulls onLevelEnum.object field.
Thanks for your feedback,
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com
On 03/21/2013 12:12 PM, Laurent Bourgès wrote:
Here is an improved patch tested on JDK7u13 and JDK8 internal
build on my
machine linux x64:
http://jmmc.fr/~bourgesl/share/webrev-8010309/
http://jmmc.fr/%7Ebourgesl/share/webrev-8010309/
FYI, I removed completely the MapInteger, Object
levelObjects and used
two arrays to perform the PlatformLogger's level (int) to
j.u.l.Level
mapping:
I decided to keep it simple as possible (no enum ...) and
used a switch
case based on current level occurences:
Hi Laurent,
In my experience enums are just the right and most compact tool
for coding such constant associations. Here's a quick try
(ripping off your optimized switch ;-):
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.01/index.html
...it adds 12 LOC to the original PlatformLogger and is 43 LOC
less tha your patch. In addition:
- only one switch instead of two (to maintain)
- no parallel IDX_ constants
What do you think?
Regards, Peter
510 /**
511 * Return the corresponding j.u.l.Level instance
512 * @param level PlatformLogger level as integer
513 * @return Object (j.u.l.Level instance) or
null if no
matching level
514 */
515 private static Object getLevel(final int level) {
516 if (levelObjects == null) {
517 return null;
518 }
519 // higher occurences first (finest, fine,
finer, info)
520 // based on isLoggable(level) calls
(03/20/2013)
521 // in jdk project
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
On 03/21/2013 03:45 PM, Peter Levart wrote:
But your concern might be correct. In my code LevelEnum is also used
from the LoggerProxy.format() method (in addition to all the places in
JavaLogger) to obtain the level name for formatting. If this method is
called the first time while LoggingSupport.isAvailable() returns false
and that happens before JavaLogger uses LevelEnum for the first time
(and at that time LoggingSupport.isAvailable() would return true),
then level objects will not be initialized correctly.
Hi Laurent,
In addition, the following sole PlatformLogger constructor:
private PlatformLogger(String name) {
if (loggingEnabled) {
this.logger = new JavaLogger(name);
} else {
this.logger = new LoggerProxy(name);
}
}
... indicates that either JavaLogger objects or LoggerProxy objects are
used but not both (since loggingEnabled is static final). So the above
concern is ungrounded.
Regards, Peter
Re: RFR-8008118
All: I modified the splitPath() procedure so that when it encounters an OOM error it frees allocated memory before exiting. Thanks! John http://cr.openjdk.java.net/~jzavgren/8008118/webrev.03/ - Original Message - From: marti...@google.com To: chris.hega...@oracle.com Cc: john.zavg...@oracle.com, core-libs-dev@openjdk.java.net Sent: Wednesday, March 20, 2013 7:32:39 PM GMT -05:00 US/Canada Eastern Subject: Re: RFR-8008118 On Wed, Mar 20, 2013 at 4:23 PM, Chris Hegarty chris.hega...@oracle.com wrote: Martin, I take your point about the other allocations, but as you say OOM is better than SEGV. So possibly good enough? Ah, Thanks Chris, I had forgotten that NEW *does* throw OOME. If NEW returns NULL, then there will be a pending OOM on the stack. good point! In that case, this is indeed a clear improvement, and I leave it to John whether to undo the allocations in splitPath before returning. Just add that space after if, please, before you submit!
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Peter,
I am convinced it is better if the LevelEnum belongs to JavaLogger class
(encapsulated so defered initialization).
Here is a typical scenario:
1 - PlatformLogger initialized (not JUL) = LoggerProxy must work so
LevelEnum can not be used and the former getLevelName() (switch case) is
required.
2 - JUL initialized: calls redirectPlatformLogger() = JavaLoggers created
= LevelEnum initialized (Level object can be retrieved from LoggingSupport)
For example, JUL can be started programmatically so the loggingEnabled flag
is not enough to initialize the LevelEnum.object at step 1 and it will be
null. Then at step 2, LevelEnum.object are null so the JavaLogger is broken.
Here is a test of this scenario:
public static void main(String[] args) {
final PlatformLogger log = PlatformLogger.getLogger(sun.awt.X11); //
1: LoggerProxy
if (log.isLoggable(PlatformLogger.INFO)) {
log.info(PlatformLogger.INFO: LoggerProxy);
}
final Logger logger = Logger.getLogger(test);
// calls PlatformLogger.redirectPlatformLoggers(); // 2 : JavaLogger
if (log.isLoggable(PlatformLogger.INFO)) {
log.info(PlatformLogger.INFO: JavaLogger);
}
logger.info(done);
}
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
Hi Laurent, Mandy,
Let me try one more time (it would be less optimal to have 2 switch
statements instead of one). Here's the 3rd webrev:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.03/index.html
The changes:
- added the comments
- made LevelEnum private (to not be tempted to be used from outside the
PlatformLogger)
- used the loggingEnabled flag as a pre-check when trying to parse the
level objects
What do you think?
Regards, Peter
On 03/21/2013 04:06 PM, Laurent Bourgès wrote:
Thanks Mandy for the clarifications !
Peter, I propose to:
- move the LevelEnum into the JavaLogger class in order to initialize it
as later as possible i.e. after j.u.l calls redirectPlatformLoggers()
- only use it in JavaLogger class (private enum) so revert changes to
PlatformLogger and LoggerProxy classes
- add few comments in the code to explain lazy initialization (see mandy's
answer)
Finally, could you keep my comment before the switch case (high occurences
first) ?
+static LevelEnum forValue(int levelValue) {
+// higher occurences first (finest, fine, finer, info)+
// based on isLoggable(level) calls (03/20/2013) +// in jdk
project only (including generated sources)
+switch (levelValue) {+case
PlatformLogger.FINEST: return FINEST; // 116 + 2257 matches in generated
files+case PlatformLogger.FINE:return FINE;// 270
+case PlatformLogger.FINER: return FINER; // 157+
case PlatformLogger.INFO:return INFO;// 39+
case PlatformLogger.WARNING: return WARNING; // 12
+case PlatformLogger.CONFIG: return CONFIG; // 6+
case PlatformLogger.SEVERE: return SEVERE; // 1+case
PlatformLogger.OFF: return OFF; // 0
+case PlatformLogger.ALL: return ALL; // 0+
default: return UNKNOWN;+}
+}
cheers,
Laurent
2013/3/21 Mandy Chung mandy.ch...@oracle.com
Laurent, Peter,
I haven't looked at the patch yet. One thing worths mentioning is that
PlatformLogger was added for the platform code to use so as to avoid the
initialization of java.util.logging since logging is not turned on by
default and that to reduce the startup overhead. In addition, it also
enables the elimination of the core classes dependency from
java.util.logging for modularization effort. Therefore the PlatformLogger
only lazily looks up the Level object when java.util.logging is present and
also has been initialized by application code.
Mandy
On 3/21/2013 7:45 AM, Peter Levart wrote:
On 03/21/2013 03:30 PM, Laurent Bourgès wrote:
Peter,
your solution looks better; I wanted my patch to be simple, efficient and
only modify the JavaLogger class (localized changes).
In your patch, I have doubts related to lazy and conditional
initialization in JavaLogger (static initialization):
if (LoggingSupport.isAvailable()) {
// initialize ...
}
In original code, if LoggingSupport.isAvailable() returned false,
levelObjects
map remained empty and consequently null was used as the level object
passed to LoggingSupport methods. In LevelEnum I try to keep this logic.
When LevelEnum is first used, it's constants are initialized and level
objects with them. If
LoggingSupport.isAvailable() returns false, level objects are initialized
to null.
I just noticed there's a bug in initialization of the LevelEnum.UNKNOWN
member constant. It should not try to parse level object. Here's an update:
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
On 03/21/2013 04:31 PM, Peter Levart wrote:
Hi Laurent, Mandy,
Let me try one more time (it would be less optimal to have 2 switch
statements instead of one). Here's the 3rd webrev:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.03/index.html
The changes:
- added the comments
- made LevelEnum private (to not be tempted to be used from outside
the PlatformLogger)
Hi Mandy,
JavaLogger and LoggerProxy are only used from within PlatformLogger.
Wouldn't it be better to declare them private? Their usage (one or the
other) depends on the 'loggingSupport' flag that is initialized with
PlatformLogger class. Usage of JavaLogger or LoggerProxy outside
PlatformLogger would be wrong, I think.
- used the loggingEnabled flag as a pre-check when trying to parse the
level objects
I think this pre-check is optimal and does not even try to load the
LoggingSupport class unless loggingEnabled is true. I tried the
following test:
public class TestLazyClassInit {
private static final boolean FALSE =
Boolean.getBoolean(non.existent.property);
static class Foo {
static {
System.out.println(Foo initialized);
}
static boolean isOk() {
return true;
}
}
public static void main(String[] args) {
if (FALSE Foo.isOk()) {
System.out.println(FALSE is true);
} else {
System.out.println(FALSE is false);
}
}
}
... and it only prints: FALSE is false...
Regards, Peter
What do you think?
Regards, Peter
On 03/21/2013 04:06 PM, Laurent Bourgès wrote:
Thanks Mandy for the clarifications !
Peter, I propose to:
- move the LevelEnum into the JavaLogger class in order to initialize
it as later as possible i.e. after j.u.l calls redirectPlatformLoggers()
- only use it in JavaLogger class (private enum) so revert changes to
PlatformLogger and LoggerProxy classes
- add few comments in the code to explain lazy initialization (see
mandy's answer)
Finally, could you keep my comment before the switch case (high
occurences first) ?
+static LevelEnum forValue(int levelValue) {
+// higher occurences first (finest, fine, finer, info)
+// based on isLoggable(level) calls (03/20/2013)
+// in jdk project only (including generated sources)
+switch (levelValue) {
+case PlatformLogger.FINEST: return FINEST; // 116 + 2257
matches in generated files
+case PlatformLogger.FINE:return FINE;// 270
+case PlatformLogger.FINER: return FINER; // 157
+case PlatformLogger.INFO:return INFO;// 39
+case PlatformLogger.WARNING: return WARNING; // 12
+case PlatformLogger.CONFIG: return CONFIG; // 6
+case PlatformLogger.SEVERE: return SEVERE; // 1
+case PlatformLogger.OFF: return OFF; // 0
+case PlatformLogger.ALL: return ALL; // 0
+default: return UNKNOWN;
+}
+}
cheers,
Laurent
2013/3/21 Mandy Chung mandy.ch...@oracle.com
mailto:mandy.ch...@oracle.com
Laurent, Peter,
I haven't looked at the patch yet. One thing worths mentioning
is that PlatformLogger was added for the platform code to use so
as to avoid the initialization of java.util.logging since logging
is not turned on by default and that to reduce the startup
overhead. In addition, it also enables the elimination of the
core classes dependency from java.util.logging for modularization
effort. Therefore the PlatformLogger only lazily looks up the
Level object when java.util.logging is present and also has been
initialized by application code.
Mandy
On 3/21/2013 7:45 AM, Peter Levart wrote:
On 03/21/2013 03:30 PM, Laurent Bourgès wrote:
Peter,
your solution looks better; I wanted my patch to be simple,
efficient and only modify the JavaLogger class (localized changes).
In your patch, I have doubts related to lazy and conditional
initialization in JavaLogger (static initialization):
if (LoggingSupport.isAvailable()) {
// initialize ...
}
In original code, if LoggingSupport.isAvailable() returned
false, levelObjects map remained empty and consequently null was
used as the level object passed to LoggingSupport methods. In
LevelEnum I try to keep this logic. When LevelEnum is first
used, it's constants are initialized and level objects with
them. If
LoggingSupport.isAvailable() returns false, level objects are
initialized to null.
I just noticed there's a bug in initialization of the
LevelEnum.UNKNOWN member constant. It should not try to parse
level object. Here's an update:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.02/index.html
But your concern might be correct.
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
You're right, loggingEnabled can change afterwards. I'll correct the
code as suggested...
Peter
On 03/21/2013 04:57 PM, Laurent Bourgès wrote:
Peter,
I am convinced it is better if the LevelEnum belongs to JavaLogger
class (encapsulated so defered initialization).
Here is a typical scenario:
1 - PlatformLogger initialized (not JUL) = LoggerProxy must work so
LevelEnum can not be used and the formergetLevelName() (switch case)
is required.
2 - JUL initialized: calls redirectPlatformLogger() = JavaLoggers
created = LevelEnum initialized (Level object can be retrieved from
LoggingSupport)
For example, JUL can be started programmatically so the loggingEnabled
flag is not enough to initialize the LevelEnum.object at step 1 and it
will be null. Then at step 2, LevelEnum.object are null so the
JavaLogger is broken.
Here is a test of this scenario:
public static void main(String[] args) {
final PlatformLogger log =
PlatformLogger.getLogger(sun.awt.X11); // 1: LoggerProxy
if (log.isLoggable(PlatformLogger.INFO)) {
log.info http://log.info(PlatformLogger.INFO: LoggerProxy);
}
final Logger logger = Logger.getLogger(test);
// calls PlatformLogger.redirectPlatformLoggers(); // 2 : JavaLogger
if (log.isLoggable(PlatformLogger.INFO)) {
log.info http://log.info(PlatformLogger.INFO: JavaLogger);
}
logger.info http://logger.info(done);
}
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com
Hi Laurent, Mandy,
Let me try one more time (it would be less optimal to have 2
switch statements instead of one). Here's the 3rd webrev:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.03/index.html
The changes:
- added the comments
- made LevelEnum private (to not be tempted to be used from
outside the PlatformLogger)
- used the loggingEnabled flag as a pre-check when trying to parse
the level objects
What do you think?
Regards, Peter
On 03/21/2013 04:06 PM, Laurent Bourgès wrote:
Thanks Mandy for the clarifications !
Peter, I propose to:
- move the LevelEnum into the JavaLogger class in order to
initialize it as later as possible i.e. after j.u.l calls
redirectPlatformLoggers()
- only use it in JavaLogger class (private enum) so revert
changes to PlatformLogger and LoggerProxy classes
- add few comments in the code to explain lazy initialization
(see mandy's answer)
Finally, could you keep my comment before the switch case (high
occurences first) ?
+static LevelEnum forValue(int levelValue) {
+// higher occurences first (finest, fine, finer, info)
+// based on isLoggable(level) calls (03/20/2013)
+// in jdk project only (including generated sources)
+switch (levelValue) {
+case PlatformLogger.FINEST: return FINEST; // 116 + 2257
matches in generated files
+case PlatformLogger.FINE:return FINE;// 270
+case PlatformLogger.FINER: return FINER; // 157
+case PlatformLogger.INFO:return INFO;// 39
+case PlatformLogger.WARNING: return WARNING; // 12
+case PlatformLogger.CONFIG: return CONFIG; // 6
+case PlatformLogger.SEVERE: return SEVERE; // 1
+case PlatformLogger.OFF: return OFF; // 0
+case PlatformLogger.ALL: return ALL; // 0
+default: return UNKNOWN;
+}
+}
cheers,
Laurent
2013/3/21 Mandy Chung mandy.ch...@oracle.com
mailto:mandy.ch...@oracle.com
Laurent, Peter,
I haven't looked at the patch yet. One thing worths
mentioning is that PlatformLogger was added for the platform
code to use so as to avoid the initialization of
java.util.logging since logging is not turned on by default
and that to reduce the startup overhead. In addition, it
also enables the elimination of the core classes dependency
from java.util.logging for modularization effort. Therefore
the PlatformLogger only lazily looks up the Level object when
java.util.logging is present and also has been initialized by
application code.
Mandy
On 3/21/2013 7:45 AM, Peter Levart wrote:
On 03/21/2013 03:30 PM, Laurent Bourgès wrote:
Peter,
your solution looks better; I wanted my patch to be simple,
efficient and only modify the JavaLogger class (localized
changes).
In your patch, I have doubts related to lazy and
conditional initialization in JavaLogger (static
initialization):
if (LoggingSupport.isAvailable()) {
// initialize ...
}
In original code, if
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Hi Laurent, Mandy,
I think I've got it now. It's a middle-ground. It would be a shame to
loose the code-reuse (one switch instead of two). So I only moved the
levelObjects to JavaLogger, not the entire LevelEnum:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.04/index.html
So this is basically similar to original code. It just uses LevelEnum
instead of java.lang.Integer and EnumMap instead of HashMap. Hopefully
these perform better. If I moved the LevelEnum into the JavaLogger, I
would have to re-instate the second switch for LoggerProxy and the
solution would not be much simpler than your patch, Laurent. This is
still just 20 LOC more than original code.
Regards, Peter
On 03/21/2013 05:09 PM, Peter Levart wrote:
You're right, loggingEnabled can change afterwards. I'll correct the
code as suggested...
Peter
On 03/21/2013 04:57 PM, Laurent Bourgès wrote:
Peter,
I am convinced it is better if the LevelEnum belongs to JavaLogger
class (encapsulated so defered initialization).
Here is a typical scenario:
1 - PlatformLogger initialized (not JUL) = LoggerProxy must work so
LevelEnum can not be used and the formergetLevelName() (switch case)
is required.
2 - JUL initialized: calls redirectPlatformLogger() = JavaLoggers
created = LevelEnum initialized (Level object can be retrieved from
LoggingSupport)
For example, JUL can be started programmatically so the
loggingEnabled flag is not enough to initialize the LevelEnum.object
at step 1 and it will be null. Then at step 2, LevelEnum.object are
null so the JavaLogger is broken.
Here is a test of this scenario:
public static void main(String[] args) {
final PlatformLogger log =
PlatformLogger.getLogger(sun.awt.X11); // 1: LoggerProxy
if (log.isLoggable(PlatformLogger.INFO)) {
log.info http://log.info(PlatformLogger.INFO: LoggerProxy);
}
final Logger logger = Logger.getLogger(test);
// calls PlatformLogger.redirectPlatformLoggers(); // 2 : JavaLogger
if (log.isLoggable(PlatformLogger.INFO)) {
log.info http://log.info(PlatformLogger.INFO: JavaLogger);
}
logger.info http://logger.info(done);
}
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com
Hi Laurent, Mandy,
Let me try one more time (it would be less optimal to have 2
switch statements instead of one). Here's the 3rd webrev:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.03/index.html
The changes:
- added the comments
- made LevelEnum private (to not be tempted to be used from
outside the PlatformLogger)
- used the loggingEnabled flag as a pre-check when trying to
parse the level objects
What do you think?
Regards, Peter
On 03/21/2013 04:06 PM, Laurent Bourgès wrote:
Thanks Mandy for the clarifications !
Peter, I propose to:
- move the LevelEnum into the JavaLogger class in order to
initialize it as later as possible i.e. after j.u.l calls
redirectPlatformLoggers()
- only use it in JavaLogger class (private enum) so revert
changes to PlatformLogger and LoggerProxy classes
- add few comments in the code to explain lazy initialization
(see mandy's answer)
Finally, could you keep my comment before the switch case (high
occurences first) ?
+static LevelEnum forValue(int levelValue) {
+// higher occurences first (finest, fine, finer, info)
+// based on isLoggable(level) calls (03/20/2013)
+// in jdk project only (including generated sources)
+switch (levelValue) {
+case PlatformLogger.FINEST: return FINEST; // 116 + 2257
matches in generated files
+case PlatformLogger.FINE:return FINE;// 270
+case PlatformLogger.FINER: return FINER; // 157
+case PlatformLogger.INFO:return INFO;// 39
+case PlatformLogger.WARNING: return WARNING; // 12
+case PlatformLogger.CONFIG: return CONFIG; // 6
+case PlatformLogger.SEVERE: return SEVERE; // 1
+case PlatformLogger.OFF: return OFF; // 0
+case PlatformLogger.ALL: return ALL; // 0
+default: return UNKNOWN;
+}
+}
cheers,
Laurent
2013/3/21 Mandy Chung mandy.ch...@oracle.com
mailto:mandy.ch...@oracle.com
Laurent, Peter,
I haven't looked at the patch yet. One thing worths
mentioning is that PlatformLogger was added for the platform
code to use so as to avoid the initialization of
java.util.logging since logging is not turned on by default
and that to reduce the startup overhead. In addition, it
also enables the elimination of the core classes dependency
from java.util.logging for modularization
Re: RFR-8008118
Please revert this formatting change:
-for (q = p; (*q != ':') (*q != '\0'); q++)
-;
+for (q = p; (*q != ':') (*q != '\0'); q++);
+
I would not introduce variable k and just count down from i, like this:
if (pathv[i] == NULL) {
for (i--; i = 0; i--)
free(pathv[i]);
return NULL;
}
which looks a little cleaner.
On Thu, Mar 21, 2013 at 8:53 AM, John Zavgren john.zavg...@oracle.comwrote:
All:
I modified the splitPath() procedure so that when it encounters an OOM
error it frees allocated memory before exiting.
Thanks!
John
http://cr.openjdk.java.net/~jzavgren/8008118/webrev.03/
- Original Message -
From: marti...@google.com
To: chris.hega...@oracle.com
Cc: john.zavg...@oracle.com, core-libs-dev@openjdk.java.net
Sent: Wednesday, March 20, 2013 7:32:39 PM GMT -05:00 US/Canada Eastern
Subject: Re: RFR-8008118
On Wed, Mar 20, 2013 at 4:23 PM, Chris Hegarty
chris.hega...@oracle.comwrote:
Martin,
I take your point about the other allocations, but as you say OOM is
better than SEGV. So possibly good enough?
Ah, Thanks Chris, I had forgotten that NEW *does* throw OOME.
If NEW returns NULL, then there will be a pending OOM on the stack.
good point!
In that case, this is indeed a clear improvement, and I leave it to John
whether to undo the allocations in splitPath before returning.
Just add that space after if, please, before you submit!
Re: RFR-8008118
On 21/03/2013 15:53, John Zavgren wrote: All: I modified the splitPath() procedure so that when it encounters an OOM error it frees allocated memory before exiting. Thanks! John http://cr.openjdk.java.net/~jzavgren/8008118/webrev.03/ Will this work with empty path components (./), I assume they need to be skipped when freeing. -Alan
Re: RFR-8008118
On Thu, Mar 21, 2013 at 10:10 AM, Martin Buchholz marti...@google.comwrote:
if (pathv[i] == NULL) {
for (i--; i = 0; i--)
free(pathv[i]);
return NULL;
}
Alan correctly points out that you can't free ./.
static const char * const cwd = ./;
...
if (pathv[i] == NULL) {
for (i--; i = 0; i--)
if (pathv[i] != cwd)
free(pathv[i]);
return NULL;
}
Manual memory management is a drag.
hg: jdk8/tl/jdk: 8009251: Add proxy handling and keep-alive fixes to jsse
Changeset: 3f8fbb0ab155 Author:robm Date: 2013-03-21 17:33 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3f8fbb0ab155 8009251: Add proxy handling and keep-alive fixes to jsse Reviewed-by: chegar ! src/share/classes/sun/net/www/http/HttpClient.java ! src/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java ! src/share/classes/sun/net/www/protocol/https/HttpsClient.java
Re: Please review: surrogate fiddle
On Thu, Mar 21, 2013 at 5:29 AM, Ulf Zibis ulf.zi...@cosoco.de wrote: Thanks! But what's about the wrong exception index reporting, e.g. 0 as out of bounds? I am not going to change the exceptions thrown from these methods, even though I agree we could do a little better if this was new code.
Re: Please review: surrogate fiddle
On Thu, Mar 21, 2013 at 5:56 AM, Ulf Zibis ulf.zi...@cosoco.de wrote: Anyway @throws would be better than @exception A JDK-wide cleanup of @exception = @throws (any many similar things) would be valuable, but I am not going to try here.
Re: RFR-8008118
On Mar 21, 10:10am, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: RFR-8008118 | Please revert this formatting change: | | -for (q = p; (*q != ':') (*q != '\0'); q++) | -; | +for (q = p; (*q != ':') (*q != '\0'); q++); | + | Stylistically I prefer: for (q = p; (*q != ':') (*q != '\0'); q++) continue; so that re-formatting accidents don't happen, and the intent is clearly communicated. christos
Re: RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK
Hi Peter, On 3/20/2013 2:33 AM, Peter Levart wrote: Hi, If I understand correctly, sun.reflect.Reflection.getCallerClass(int) will not be usable by non-system code any more. I know it's not a public API, but it's useful in some situations (not security-related) and there's no public API for that functionality. Do you have any example of existing use of Reflection.getCallerClass to find immediate caller class? or does it need to walk the stack? That'd be useful. Is there a reason to not unofficially support also classes with @CS annotated methods and which are not loaded by bootstrap or extension class-loader ? We expect that most of the caller-sensitive cases are in the JDK and should be rare for non-system libraries to have caller-sensitive code. It'd be good to understand the use cases and the requirements to determine the appropriate support for it. Mandy Regards, Peter On 03/15/2013 04:31 AM, Christian Thalinger wrote: [This is the HotSpot part of JEP 176] http://cr.openjdk.java.net/~twisti/7198429 7198429: need checked categorization of caller-sensitive methods in the JDK Reviewed-by: More information in JEP 176: http://openjdk.java.net/jeps/176 src/share/vm/ci/ciMethod.cpp src/share/vm/ci/ciMethod.hpp src/share/vm/classfile/classFileParser.cpp src/share/vm/classfile/classFileParser.hpp src/share/vm/classfile/javaClasses.hpp src/share/vm/classfile/systemDictionary.hpp src/share/vm/classfile/vmSymbols.hpp src/share/vm/oops/method.cpp src/share/vm/oops/method.hpp src/share/vm/opto/library_call.cpp src/share/vm/prims/jvm.cpp src/share/vm/prims/methodHandles.cpp src/share/vm/prims/unsafe.cpp src/share/vm/runtime/vframe.cpp src/share/vm/runtime/vframe.hpp
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Peter,
My last idea for today:
Consider the object field of the LevelEnum as a mutable field (non final),
JavaLogger initialization can then set its value during its initialization
(j.u.l available).
Doing so, it will avoid any Map.get() in isLoggable(int) to improve
performance as done in my patch.
I will test this solution soon...
My previous message I forgot to send to the mailing list:
my last two cents:
- I prefer having the object()and forObject() methods in the JavaLogger
class because it reduces the risk to use it elsewhere and it avoids double
indirection: JavaLogger calls these methods that call back JavaLogger to
access its fields ... too obfuscated for me
+
+// the following two methods are (and should only be) called
from JavaLogger...
+
+Object object() {
+return JavaLogger.levelObjects.get(this);
+}
+
+static LevelEnum forObject(Object levelObject) {
+LevelEnum levelEnum = JavaLogger.levelEnums.get(levelObject);
+return levelEnum == null ? UNKNOWN : levelEnum;
+}
+}
- take the opportunity to replace repeated lines in
LoggerProxy.doLog() methods by isLoggable(level):
if (level levelValue || levelValue == OFF) {
return;
}
Great job anyway,
Cheers,
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
Hi Laurent, Mandy,
I think I've got it now. It's a middle-ground. It would be a shame to
loose the code-reuse (one switch instead of two). So I only moved the
levelObjects to JavaLogger, not the entire LevelEnum:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.04/index.html
So this is basically similar to original code. It just uses LevelEnum
instead of java.lang.Integer and EnumMap instead of HashMap. Hopefully
these perform better. If I moved the LevelEnum into the JavaLogger, I would
have to re-instate the second switch for LoggerProxy and the solution would
not be much simpler than your patch, Laurent. This is still just 20 LOC
more than original code.
Regards, Peter
On 03/21/2013 05:09 PM, Peter Levart wrote:
You're right, loggingEnabled can change afterwards. I'll correct the code
as suggested...
Peter
On 03/21/2013 04:57 PM, Laurent Bourgès wrote:
Peter,
I am convinced it is better if the LevelEnum belongs to JavaLogger class
(encapsulated so defered initialization).
Here is a typical scenario:
1 - PlatformLogger initialized (not JUL) = LoggerProxy must work so
LevelEnum can not be used and the former getLevelName() (switch case) is
required.
2 - JUL initialized: calls redirectPlatformLogger() = JavaLoggers created
= LevelEnum initialized (Level object can be retrieved from LoggingSupport)
For example, JUL can be started programmatically so the loggingEnabled
flag is not enough to initialize the LevelEnum.object at step 1 and it
will be null. Then at step 2, LevelEnum.object are null so the JavaLogger
is broken.
Here is a test of this scenario:
public static void main(String[] args) {
final PlatformLogger log = PlatformLogger.getLogger(sun.awt.X11); //
1: LoggerProxy
if (log.isLoggable(PlatformLogger.INFO)) {
log.info(PlatformLogger.INFO: LoggerProxy);
}
final Logger logger = Logger.getLogger(test);
// calls PlatformLogger.redirectPlatformLoggers(); // 2 : JavaLogger
if (log.isLoggable(PlatformLogger.INFO)) {
log.info(PlatformLogger.INFO: JavaLogger);
}
logger.info(done);
}
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
Hi Laurent, Mandy,
Let me try one more time (it would be less optimal to have 2 switch
statements instead of one). Here's the 3rd webrev:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.03/index.html
The changes:
- added the comments
- made LevelEnum private (to not be tempted to be used from outside the
PlatformLogger)
- used the loggingEnabled flag as a pre-check when trying to parse the
level objects
What do you think?
Regards, Peter
On 03/21/2013 04:06 PM, Laurent Bourgès wrote:
Thanks Mandy for the clarifications !
Peter, I propose to:
- move the LevelEnum into the JavaLogger class in order to initialize it
as later as possible i.e. after j.u.l calls redirectPlatformLoggers()
- only use it in JavaLogger class (private enum) so revert changes to
PlatformLogger and LoggerProxy classes
- add few comments in the code to explain lazy initialization (see
mandy's answer)
Finally, could you keep my comment before the switch case (high
occurences first) ?
+static LevelEnum forValue(int levelValue) {
+// higher occurences first (finest, fine, finer, info)+
// based on isLoggable(level) calls (03/20/2013) +// in jdk
project only (including generated sources)
+switch (levelValue) {+case
PlatformLogger.FINEST: return FINEST; // 116 + 2257 matches in generated
files+
Re: RFR-8008118
All: How does this look? 1.) I reverted the for statement formatting change. 2.) I removed the goto statement and inlined some code instead. 3.) I checked to make sure that we're not freeing memory that we didn't actually allocate. (Path vector elements that are empty.) http://cr.openjdk.java.net/~jzavgren/8008118/webrev.04/ John - Original Message - From: chris...@zoulas.com To: marti...@google.com, john.zavg...@oracle.com Cc: core-libs-dev@openjdk.java.net Sent: Thursday, March 21, 2013 2:00:10 PM GMT -05:00 US/Canada Eastern Subject: Re: RFR-8008118 On Mar 21, 10:10am, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: RFR-8008118 | Please revert this formatting change: | | -for (q = p; (*q != ':') (*q != '\0'); q++) | -; | +for (q = p; (*q != ':') (*q != '\0'); q++); | + | Stylistically I prefer: for (q = p; (*q != ':') (*q != '\0'); q++) continue; so that re-formatting accidents don't happen, and the intent is clearly communicated. christos
Re: RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK
On 03/21/2013 07:11 PM, Mandy Chung wrote: Hi Peter, On 3/20/2013 2:33 AM, Peter Levart wrote: Hi, If I understand correctly, sun.reflect.Reflection.getCallerClass(int) will not be usable by non-system code any more. I know it's not a public API, but it's useful in some situations (not security-related) and there's no public API for that functionality. Do you have any example of existing use of Reflection.getCallerClass to find immediate caller class? or does it need to walk the stack? That'd be useful. I have seen a utility that uses it to establish the context (package and ClassLoader) of where to start searching for resources for GUI components construction. And a utility that wraps Class.forName(String) - specifying Class.forName(name, true, Reflection.getCallerClass(2).getClassLoader()) instead to use the caller's class loader to load the class... Usually it is only necessary to get the immediate caller. Is there a reason to not unofficially support also classes with @CS annotated methods and which are not loaded by bootstrap or extension class-loader ? We expect that most of the caller-sensitive cases are in the JDK and should be rare for non-system libraries to have caller-sensitive code. It'd be good to understand the use cases and the requirements to determine the appropriate support for it. Ok, but why limit? Is it an optimization? Is it for security? For example: one might think that creating an anonymous class and holding a j.l.Class reference somewhere safe is enough for safety, so the class itself could have public API. Now if an untrusted callback object is passed to the code of such class and it is invoked from within that code, the anonymous class can get revealed to the untrusted code which can use it's public API to invoke it. Are there any other implications of allowing non-system code to get the caller? Regards, Peter Mandy Regards, Peter On 03/15/2013 04:31 AM, Christian Thalinger wrote: [This is the HotSpot part of JEP 176] http://cr.openjdk.java.net/~twisti/7198429 7198429: need checked categorization of caller-sensitive methods in the JDK Reviewed-by: More information in JEP 176: http://openjdk.java.net/jeps/176 src/share/vm/ci/ciMethod.cpp src/share/vm/ci/ciMethod.hpp src/share/vm/classfile/classFileParser.cpp src/share/vm/classfile/classFileParser.hpp src/share/vm/classfile/javaClasses.hpp src/share/vm/classfile/systemDictionary.hpp src/share/vm/classfile/vmSymbols.hpp src/share/vm/oops/method.cpp src/share/vm/oops/method.hpp src/share/vm/opto/library_call.cpp src/share/vm/prims/jvm.cpp src/share/vm/prims/methodHandles.cpp src/share/vm/prims/unsafe.cpp src/share/vm/runtime/vframe.cpp src/share/vm/runtime/vframe.hpp
Re: RFR-8008118
On 21/03/2013 18:36, John Zavgren wrote:
All:
How does this look?
1.) I reverted the for statement formatting change.
2.) I removed the goto statement and inlined some code instead.
3.) I checked to make sure that we're not freeing memory that we didn't
actually allocate. (Path vector elements that are empty.)
http://cr.openjdk.java.net/~jzavgren/8008118/webrev.04/
John
On line 262 then shouldn't this be pathv[i] = cwd;
The recovery when NEW fails looks right to me although while (--i = 0)
{ ... } might be clearer.
-Alan.
Re: RFR-8008118
On Mar 21, 11:36am, john.zavg...@oracle.com (John Zavgren) wrote: -- Subject: Re: RFR-8008118 | All: | | How does this look? | 1.) I reverted the for statement formatting change. Not exactly. Now it is indented incorrectly. | 2.) I removed the goto statement and inlined some code instead. I prefer to write simpler code that works with both signed and unsigned indexes: +while (i 0) +if (pathv[--i] != cwd) +free(pathv[i]); + | 3.) I checked to make sure that we're not freeing memory that we didn't act= | ually allocate. (Path vector elements that are empty.) You are still using the ./ string literal constant in the code so you'll end up freeing it (the compiler will prolly merge the two instances and you'll get lucky but it is semantically incorrect). christos
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
On 03/21/2013 07:25 PM, Laurent Bourgès wrote:
Peter,
My last idea for today:
Consider the object field of the LevelEnum as a mutable field (non
final), JavaLogger initialization can then set its value during its
initialization (j.u.l available).
Doing so, it will avoid any Map.get() in isLoggable(int) to improve
performance as done in my patch.
Good idea. And it's thread-safe too I think, since the static
initializer HB any instance methods can run and the field is only
accessed from JavaLogger's instance methods
Here's the new rev:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.05/index.html
I removed the object() and forObject() methods and access the field
directly. I also renamed it to something more significant. forObject()
is not necessary, since it only had one call site and I just inlined it.
I also took the opportunity. Now we're back to original + 9 LOC ...
Regards, Peter
I will test this solution soon...
My previous message I forgot to send to the mailing list:
my last two cents:
- I prefer having theobject()and forObject() methods in the
JavaLogger class because it reduces the risk to use it elsewhere and
it avoids double indirection: JavaLogger calls these methods that call
back JavaLogger to access its fields ... too obfuscated for me
+
+// the following two methods are (and should only be) called from
JavaLogger...
+
+Object object() {
+return JavaLogger.levelObjects.get(this);
+}
+
+static LevelEnum forObject(Object levelObject) {
+LevelEnum levelEnum = JavaLogger.levelEnums.get(levelObject);
+return levelEnum == null ? UNKNOWN : levelEnum;
+}
+}
- take the opportunity to replace repeated lines in LoggerProxy.doLog() methods
by isLoggable(level):
if (level levelValue || levelValue == OFF) {
return;
}
Great job anyway,
Cheers,
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com
Hi Laurent, Mandy,
I think I've got it now. It's a middle-ground. It would be a shame
to loose the code-reuse (one switch instead of two). So I only
moved the levelObjects to JavaLogger, not the entire LevelEnum:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.04/index.html
So this is basically similar to original code. It just uses
LevelEnum instead of java.lang.Integer and EnumMap instead of
HashMap. Hopefully these perform better. If I moved the LevelEnum
into the JavaLogger, I would have to re-instate the second switch
for LoggerProxy and the solution would not be much simpler than
your patch, Laurent. This is still just 20 LOC more than original
code.
Regards, Peter
On 03/21/2013 05:09 PM, Peter Levart wrote:
You're right, loggingEnabled can change afterwards. I'll correct
the code as suggested...
Peter
On 03/21/2013 04:57 PM, Laurent Bourgès wrote:
Peter,
I am convinced it is better if the LevelEnum belongs to
JavaLogger class (encapsulated so defered initialization).
Here is a typical scenario:
1 - PlatformLogger initialized (not JUL) = LoggerProxy must
work so LevelEnum can not be used and the formergetLevelName()
(switch case) is required.
2 - JUL initialized: calls redirectPlatformLogger() =
JavaLoggers created = LevelEnum initialized (Level object can
be retrieved from LoggingSupport)
For example, JUL can be started programmatically so the
loggingEnabled flag is not enough to initialize the
LevelEnum.object at step 1 and it will be null. Then at step 2,
LevelEnum.object are null so the JavaLogger is broken.
Here is a test of this scenario:
public static void main(String[] args) {
final PlatformLogger log =
PlatformLogger.getLogger(sun.awt.X11); // 1: LoggerProxy
if (log.isLoggable(PlatformLogger.INFO)) {
log.info http://log.info(PlatformLogger.INFO: LoggerProxy);
}
final Logger logger = Logger.getLogger(test);
// calls PlatformLogger.redirectPlatformLoggers(); // 2 :
JavaLogger
if (log.isLoggable(PlatformLogger.INFO)) {
log.info http://log.info(PlatformLogger.INFO: JavaLogger);
}
logger.info http://logger.info(done);
}
Laurent
2013/3/21 Peter Levart peter.lev...@gmail.com
mailto:peter.lev...@gmail.com
Hi Laurent, Mandy,
Let me try one more time (it would be less optimal to have 2
switch statements instead of one). Here's the 3rd webrev:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.03/index.html
The changes:
- added the comments
- made LevelEnum private (to not be tempted to be used from
outside the PlatformLogger)
- used the loggingEnabled flag as a pre-check when trying to
parse the level objects
What do you think?
Re: Review request 8010416 Addition of Driver.deregisterDriver
On 21/03/2013 13:36, Lance Andersen - Oracle wrote: Need a reviewer for 8010416, addition of Driver.deregisterDriver. The webrev can be found at http://cr.openjdk.java.net/~lancea/8010416/webrev.00/. If I read this correctly then the intention is that the Driver is notified and so that it gets a chance to clean-up when DriverManager.deregisterDriver is called to deregister it. I can see that being useful but I don't think I understand what releases any resources means. Does it means that the Driver can no longer be used or does it mean that it should only release resources associated with its registration with the driver manager? I think this is distinction is important because the former means that that Driver might need to extend AutoCloseable and define the behavior for the case that its methods are invoked after it has been closed. In addition, I think the default method isn't giving you the complete solution because existing drivers will continue to work after their deregisterDriver is invoked. The other concern is that Driver.deregisterDriver can be invoked directly and this makes it possible for DriverManager to hand out Drivers that have their resources released and I don't think this is the intention. I'm also concerned about the default method specifying the SecurityException as that requires all drivers to implement this. Ideally the permission check is in one place, DriverManager. So overall I think this one will require a bit more consideration. One idea to try is to create AbstractDriver with a protected deregister method that is a no-op. Drivers would extend this if they have clean-up to do. There may be other solutions, this is just one to try but would force Drivers to extend this instead of implementing Driver direction. There may be other ideas to try too (I just mention this one). The main thing is that all the implications are understood as I think this is a tricky one to get right. -Alan.
Re: RFR 8009517: Disable fatal compiler warning in the old build
David, Plugin's integration next week is now also impacted since they also have to use the old build environment, so it's time to resolve this. So here's an update/proposal/webrev. The original bug filed by David Katleman was: 8009517: build-infra: jdk8: -Werror not being applied to nio builds and I filed: 8010434: Old build environment no longer builds. I've closed mine, and will integrate against the former. In my partial (JDK) build using the old mechanism and the current TL JDK/langtools gate, only two changes are now necessary. I propose we disable -deprecation in jdk/makejavax/others, and -overrides in make/com/sun/org/apache/xml. The first *LIKELY* crept in due to a recent change in the Base64 code which no longer implicitly compiles sun/misc/CharacterDecoder.java without -Werror active. The original deprecation issue needs to be addressed sometime by the responsible team, but that will avoid RE having to hand edit to the Makefiles just to build JCE. The second change is due to the compiler change with hashCode/equals. The codereview is here: http://cr.openjdk.java.net/~wetmore/8009517/webrev.00/ I plan to push through the deploy gate, as they have an integration next week. Thomas Ng will do the push for us. Any objections, please speak now. Brad On 3/18/2013 6:29 PM, Brad Wetmore wrote: Sorry for the delay in response, I've been pulled in yet another direction, and this has come back up in priority. On 3/9/2013 12:11 AM, Chris Hegarty wrote: I agree about warning creeping problems. This is a temporary solution, we should soon be fixing the underlying hashcode/equals problems...but... Your temporary solution, -overrides, is just that. It will enable the old build to complete today, but it could fail at any point in the future, as the code changes. Correct. As it stands today, a recent change now requires *BOTH* overrides/deprecation in order to get a complete MASTER build using the old build system. [brwetmor@flicker-vm1] 222 hg diff common/shared/Defs-java.gmk diff --git a/make/common/shared/Defs-java.gmk b/make/common/shared/Defs-java.gmk --- a/make/common/shared/Defs-java.gmk +++ b/make/common/shared/Defs-java.gmk @@ -127,7 +127,7 @@ endif # TODO: Workaround for CR 7063027. Remove -path eventually. -JAVAC_LINT_OPTIONS += -Xlint:-path +JAVAC_LINT_OPTIONS += -Xlint:-path,-overrides,-deprecation JAVACFLAGS += $(JAVAC_LINT_OPTIONS) For example, java.net is currently warning free, in the old it compiles with fatal warnings enabled. Lets say, in a moment of madness, I add a dependency from java.net.Socket to say java.awt.RenderingHints.Key ( or any class that produces warnings when compiled. I run the new build, all is fine. Push the changes. Now someone else sync's up, but need to build using the old build. If the new dependent class is not already compiled before java.net.Socket gets compiled, it will be compiled implicitly. It's warnings will cause the compile to fail, and the old build will fail. Or much simpler, anyone could write sloppy code with warnings, the new build will suppress them, and they won't notice. Push this code, and the old build will fail if is explicitly, or implicitly, compiles this code with -Werror enabled. Exactly. Our formerly clean code now requires disabling of two Lint options, but the new build is happy just to report the warning. The old build crashes on the warning. Our options for the old build system are: 1. disable the warning for overrides/deprecation, keep -Werror (my preferred since these are minor warnings.) 2. Somehow disable -Werror on these new directories that are now failing. (more work to figure out, but also acceptable) 3. Fix the warnings. (I don't have cycles to drive a rewrite of use of deprecated code and/or add missing equals/hashcode that the recent javac changes exposed.) We spent a lot of time cleaning up many directories, seems a shame to start allowing non-fatal warnings to come back into previously clean code because people aren't taking the time to fix new warnings as they are introduced. I personally spent several weeks over the past number of years fixing warnings and reviewing warning cleanup webrevs from others. I took much pride in keeping certain areas warnings free. It is with great regret that I propose to disable fatal warnings in the old build, but I felt this the best/safest option. I heard much annoyance and frustration from others about hitting seemingly random errors with the old build recently. This is the only sure way to avoid that. The new builds will still warn, but the old builds will still fail for all but these override problems. Yes, you lose the warnings in the old, but seems better than completely shutting off erroring. I'm ok with that, if others are. To clarify, I think you are suggesting that we keep the old build as it, with -overrides, and now ,-deprecation :( and use it periodically as a way of tracking new warnings
Re: RFR 8009517: Disable fatal compiler warning in the old build
This looks fine. Get it in before something else changes! Mike On Mar 21 2013, at 15:12 , Brad Wetmore wrote: David, Plugin's integration next week is now also impacted since they also have to use the old build environment, so it's time to resolve this. So here's an update/proposal/webrev. The original bug filed by David Katleman was: 8009517: build-infra: jdk8: -Werror not being applied to nio builds and I filed: 8010434: Old build environment no longer builds. I've closed mine, and will integrate against the former. In my partial (JDK) build using the old mechanism and the current TL JDK/langtools gate, only two changes are now necessary. I propose we disable -deprecation in jdk/makejavax/others, and -overrides in make/com/sun/org/apache/xml. The first *LIKELY* crept in due to a recent change in the Base64 code which no longer implicitly compiles sun/misc/CharacterDecoder.java without -Werror active. The original deprecation issue needs to be addressed sometime by the responsible team, but that will avoid RE having to hand edit to the Makefiles just to build JCE. The second change is due to the compiler change with hashCode/equals. The codereview is here: http://cr.openjdk.java.net/~wetmore/8009517/webrev.00/ I plan to push through the deploy gate, as they have an integration next week. Thomas Ng will do the push for us. Any objections, please speak now. Brad On 3/18/2013 6:29 PM, Brad Wetmore wrote: Sorry for the delay in response, I've been pulled in yet another direction, and this has come back up in priority. On 3/9/2013 12:11 AM, Chris Hegarty wrote: I agree about warning creeping problems. This is a temporary solution, we should soon be fixing the underlying hashcode/equals problems...but... Your temporary solution, -overrides, is just that. It will enable the old build to complete today, but it could fail at any point in the future, as the code changes. Correct. As it stands today, a recent change now requires *BOTH* overrides/deprecation in order to get a complete MASTER build using the old build system. [brwetmor@flicker-vm1] 222 hg diff common/shared/Defs-java.gmk diff --git a/make/common/shared/Defs-java.gmk b/make/common/shared/Defs-java.gmk --- a/make/common/shared/Defs-java.gmk +++ b/make/common/shared/Defs-java.gmk @@ -127,7 +127,7 @@ endif # TODO: Workaround for CR 7063027. Remove -path eventually. -JAVAC_LINT_OPTIONS += -Xlint:-path +JAVAC_LINT_OPTIONS += -Xlint:-path,-overrides,-deprecation JAVACFLAGS += $(JAVAC_LINT_OPTIONS) For example, java.net is currently warning free, in the old it compiles with fatal warnings enabled. Lets say, in a moment of madness, I add a dependency from java.net.Socket to say java.awt.RenderingHints.Key ( or any class that produces warnings when compiled. I run the new build, all is fine. Push the changes. Now someone else sync's up, but need to build using the old build. If the new dependent class is not already compiled before java.net.Socket gets compiled, it will be compiled implicitly. It's warnings will cause the compile to fail, and the old build will fail. Or much simpler, anyone could write sloppy code with warnings, the new build will suppress them, and they won't notice. Push this code, and the old build will fail if is explicitly, or implicitly, compiles this code with -Werror enabled. Exactly. Our formerly clean code now requires disabling of two Lint options, but the new build is happy just to report the warning. The old build crashes on the warning. Our options for the old build system are: 1. disable the warning for overrides/deprecation, keep -Werror (my preferred since these are minor warnings.) 2. Somehow disable -Werror on these new directories that are now failing. (more work to figure out, but also acceptable) 3. Fix the warnings. (I don't have cycles to drive a rewrite of use of deprecated code and/or add missing equals/hashcode that the recent javac changes exposed.) We spent a lot of time cleaning up many directories, seems a shame to start allowing non-fatal warnings to come back into previously clean code because people aren't taking the time to fix new warnings as they are introduced. I personally spent several weeks over the past number of years fixing warnings and reviewing warning cleanup webrevs from others. I took much pride in keeping certain areas warnings free. It is with great regret that I propose to disable fatal warnings in the old build, but I felt this the best/safest option. I heard much annoyance and frustration from others about hitting seemingly random errors with the old build recently. This is the only sure way to avoid that. The new builds will still warn, but the old builds will still fail for all but these override problems. Yes, you lose the warnings in the old, but seems better than completely shutting off erroring.
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Hi Peter, Laurent, Good work and I like the solution. On 3/21/13 9:00 AM, Peter Levart wrote: Hi Mandy, JavaLogger and LoggerProxy are only used from within PlatformLogger. Wouldn't it be better to declare them private? Their usage (one or the other) depends on the 'loggingSupport' flag that is initialized with PlatformLogger class. Usage of JavaLogger or LoggerProxy outside PlatformLogger would be wrong, I think. Yes JavaLogger and LoggerProxy classes can be made private. Here's the new rev: http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.05/index.html I removed the object() and forObject() methods and access the field directly. I also renamed it to something more significant. forObject() is not necessary, since it only had one call site and I just inlined it. I also took the opportunity. Now we're back to original + 9 LOC ... I looked at it but not in great detail. In general it's very good clean up. The comment in line 124-132 is good information and since the code is evolving, I would suggest to take those count out. valueOf is one common method name to find an instance of a given value. Perhaps LevelEnum.forValue can be renamed to LevelEnum.valueOf(int)? It seems to be useful to add a static method LevelEnum.getLevel(int) to replace the calls to LevelEnum.forValue(newLevel).julLevel. The tests are in jdk/test/java/util/logging and jdk/test/sun/util/logging. It'd be good to check if you want to extend jdk/test/sun/util/logging/PlatformLoggerTest.java to cover all levels (it's currently already checking several). Thanks Mandy
