FYI: Fastest UFT-8 - UNICODE converter as state machine.
http://bjoern.hoehrmann.de/utf-8/decoder/dfa/ Verified on full UNICODE set. Regards, -uta
Re: RFR 8009517: Disable fatal compiler warning in the old build
On 21/03/2013 22:12, Brad Wetmore wrote: : 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. No objection here but just to mention that since you need to set NEWBUILD=false then it might not be too much extra to also set JAVAC_WARNINGS_FATAL=false. In any case, I think it would be desirable if there was a retirement date set for the old build so that the remaining users (I assume very few at this point) have something to aim for. -Alan.
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Hi Mandy Peter,
Here is an update patch applying mandy's suggestions:
http://jmmc.fr/~bourgesl/share/webrev-2-8010309/
Good work and I like the solution.
Thanks.
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.
Done.
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.
Fixed.
valueOf is one common method name to find an instance of a given value.
Perhaps LevelEnum.forValue can be renamed to LevelEnum.valueOf(int)?
Done.
It seems to be useful to add a static method LevelEnum.getLevel(int) to
replace the calls to LevelEnum.forValue(newLevel).julLevel.
I agree I prefer using methods (field encapsulation) so I added:
+/**
+ * java.util.logging.Level optionally initialized in
JavaLogger's static initializer
+ * and USED ONLY IN JavaLogger (only once java.util.logging
is available and enabled)
+ */
+private Object julLevel;
+
+void setJulLevel(Object julLevel) {
+this.julLevel = julLevel;
+}
+
+Object getJulLevel() {
+return this.julLevel;
+}
+
+/**
+ * Return the java.util.logging.Level used only in JavaLogger
+ * @param levelValue PlatformLogger level as int
+ * @return java.util.logging.Level or null if
java.util.logging is not available and disabled
+ */
+static Object getJulLevel(int levelValue) {
+return valueOf(levelValue).getJulLevel();
+}
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).
I will now have a look ...
Regards,
Laurent
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
On 03/22/2013 10:05 AM, Laurent Bourgès wrote:
Hi Mandy Peter,
Here is an update patch applying mandy's suggestions:
http://jmmc.fr/~bourgesl/share/webrev-2-8010309/
http://jmmc.fr/%7Ebourgesl/share/webrev-2-8010309/
You've beaten me! I have been preparing them too ;-) ...
https://dl.dropboxusercontent.com/u/101777488/jdk8-tl/PlatformLogger/webrev.06/index.html
I also did some some renames, that I think make the code more consistent:
- LevelEnum - Level (the code is not dependent on
java.util.logging.Level, so the name can be reused, its private anyway)
- julLevel - javaLevel (javaLevel / JavaLogger)
- LevelEnum.forValue - Level.valueOf (Mandy)
- JavaLogger.julLevelToEnum - JavaLogger.javaLevelToLevel
Other changes (to webrev.05):
- removed the occurrence counts in switch comments (as per Mandy's
suggestion)
- made LoggerProxy and JavaLogger private
- fixed double-read of volatile LoggerProxy.levelValue in
LoggerProxy.isLoggable()
- added static Level.javaLevel(int value) shortcut (Mandy)
I also updated the test to exercise the correctness of mappings.
Have I forgotten something?
Regards, Peter
Good work and I like the solution.
Thanks.
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.
Done.
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.
Fixed.
valueOf is one common method name to find an instance of a given
value. Perhaps LevelEnum.forValue can be renamed to
LevelEnum.valueOf(int)?
Done.
It seems to be useful to add a static method
LevelEnum.getLevel(int) to replace the calls to
LevelEnum.forValue(newLevel).julLevel.
I agree I prefer using methods (field encapsulation) so I added:
+/**
+ * java.util.logging.Level optionally initialized in JavaLogger's
static initializer
+ * and USED ONLY IN JavaLogger (only once java.util.logging is
available and enabled)
+ */
+private Object julLevel;
+
+void setJulLevel(Object julLevel) {
+this.julLevel = julLevel;
+}
+
+Object getJulLevel() {
+return this.julLevel;
+}
+
+/**
+ * Return the java.util.logging.Level used only in JavaLogger
+ * @param levelValue PlatformLogger level as int
+ * @return java.util.logging.Level or null if java.util.logging is not
available and disabled
+ */
+static Object getJulLevel(int levelValue) {
+return valueOf(levelValue).getJulLevel();
+}
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).
I will now have a look ...
Regards,
Laurent
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Peter, You've beaten me! I have been preparing them too ;-) ... Ok I definitely stop working on the code and let you do it. I also did some some renames, that I think make the code more consistent: - LevelEnum - Level (the code is not dependent on java.util.logging.Level, so the name can be reused, its private anyway) - julLevel - javaLevel (javaLevel / JavaLogger) - LevelEnum.forValue - Level.valueOf (Mandy) - JavaLogger.julLevelToEnum - JavaLogger.javaLevelToLevel For consistency and clarity, I would prefer having following conventions: - int levelValue (= PlatformLevel as int) and not int level (conflict with Level enum ...) - julLevel / julLogger: more explicit than javaLevel / javaLogger (java means everything ... but jul means java.util.logging and javaLogger is in conflict with JavaLogger class) Other changes (to webrev.05): - removed the occurrence counts in switch comments (as per Mandy's suggestion) - made LoggerProxy and JavaLogger private - fixed double-read of volatile LoggerProxy.levelValue in LoggerProxy.isLoggable() - added static Level.javaLevel(int value) shortcut (Mandy) I also updated the test to exercise the correctness of mappings. Well done. cheers, Laurent
hg: jdk8/tl/jdk: 8010531: Add BadKdc* tests to problem list for solaris-sparcv9
Changeset: 93cd7052d306 Author:weijun Date: 2013-03-22 19:59 +0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/93cd7052d306 8010531: Add BadKdc* tests to problem list for solaris-sparcv9 Reviewed-by: alanb ! test/ProblemList.txt
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
On 03/22/2013 11:34 AM, Laurent Bourgès wrote: Peter, You've beaten me! I have been preparing them too ;-) ... Ok I definitely stop working on the code and let you do it. Ok. I'll make the final touches, incorporating also comments and changes from your code... I also did some some renames, that I think make the code more consistent: - LevelEnum - Level (the code is not dependent on java.util.logging.Level, so the name can be reused, its private anyway) - julLevel - javaLevel (javaLevel / JavaLogger) - LevelEnum.forValue - Level.valueOf (Mandy) - JavaLogger.julLevelToEnum - JavaLogger.javaLevelToLevel For consistency and clarity, I would prefer having following conventions: - int levelValue (= PlatformLevel as int) and not int level (conflict with Level enum ...) I think that PlatformLogger public API should stay as is. Public method's parameter names are just called 'level' and values of public constants are expected to be passed to them. There are only two places where 'level' is the name of a local variable of type Level (and not int) and at both places there is not conflict, since there's no 'int level' variable in scope. I renamed LevelEnum to Level because the following most frequently used pattern looks strange: LevelEnum.javaLevel(int) Neither parameter nor the result has anything to do with LevelEnum directly. But if we move the javaLevel(int) method out of the Level enum into the JavaLogger, then Level can be renamed back to LevelEnum (or anything else?). - julLevel / julLogger: more explicit than javaLevel / javaLogger (java means everything ... but jul means java.util.logging and javaLogger is in conflict with JavaLogger class) But javaLogger is not in conflict with javaLevel. I suggest renaming JavaLoger class to JavaLoggerProxy, so we would have: Object javaLogger // holding java.util.logging.Logger instance Object javaLevel// holding java.util.logging.Level instance class JavaLoggerProxy // a specialization of LoggerProxy, delegating to javaLogger If 'jul' is a prefered prefix to 'java', I suggest renaming all 3: julLogger, julLevel, JulLoggerProxy. I don't have a preference for either, so perhaps Mandy, Laurent or anybody else can express them... Regards, Peter Other changes (to webrev.05): - removed the occurrence counts in switch comments (as per Mandy's suggestion) - made LoggerProxy and JavaLogger private - fixed double-read of volatile LoggerProxy.levelValue in LoggerProxy.isLoggable() - added static Level.javaLevel(int value) shortcut (Mandy) I also updated the test to exercise the correctness of mappings. Well done. cheers, Laurent
hg: jdk8/tl/langtools: 5 new changesets
Changeset: cc38a6723663 Author:mcimadamore Date: 2013-03-22 12:38 + URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/cc38a6723663 8009649: Lambda back-end should generate invokespecial for method handles referring to private instance methods Summary: Private lambda methods should be accessed through invokespecial Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java + test/tools/javac/lambda/bytecode/TestLambdaBytecode.java Changeset: f3814edefb33 Author:mcimadamore Date: 2013-03-22 12:39 + URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/f3814edefb33 8010101: Intersection type cast issues redundant unchecked warning Summary: Code for checking intersection type cast is incorrectly swapping operands, leading to spurious warnings Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/code/Types.java + test/tools/javac/lambda/Intersection02.java + test/tools/javac/lambda/Intersection02.out Changeset: b6cf07c54c29 Author:mcimadamore Date: 2013-03-22 12:41 + URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/b6cf07c54c29 8009820: AssertionError when compiling java code with two identical static imports Summary: Speculative attribution is carried out twice with same method symbol in case of static imports Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/comp/DeferredAttr.java + test/tools/javac/lambda/DoubleStaticImport.java Changeset: c6728c9addff Author:mcimadamore Date: 2013-03-22 12:43 + URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/c6728c9addff 8010303: Graph inference: missing incorporation step causes spurious inference error Summary: Multiple equality constraints on inference vars are not used to generate new inference constraints Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/code/Types.java ! src/share/classes/com/sun/tools/javac/comp/Infer.java ! test/tools/javac/lambda/TargetType28.out + test/tools/javac/lambda/TargetType67.java + test/tools/javac/lambda/TargetType68.java + test/tools/javac/lambda/TargetType69.java Changeset: 5da12e8a59ba Author:mcimadamore Date: 2013-03-22 12:44 + URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/5da12e8a59ba 8010387: Javac crashes when diagnostic mentions anonymous inner class' type variables Summary: Rich formatter doesn't preprocess supertypes of an anonymous inner class Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/util/RichDiagnosticFormatter.java + test/tools/javac/Diagnostics/8010387/T8010387.java + test/tools/javac/Diagnostics/8010387/T8010387.out
Re: RFR-8008118
Hi John, in the cleanup, would it be necessary to free the allocated pathv array also, prior to return? if I'm not misreading. Also, rather than storing the pointer to the string literal, malloc (NEW) and memcpy as per the other entries. This will avoid potential conflict should other code attempt to release the pathv, which would be unaware of it containing the string literal. (Who cleans up pathv after it has been used?) regards Mark 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 - 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: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Ok, Lauret, Mandy, Here are the final touches: https://dl.dropboxusercontent.com/u/101777488/jdk8-tl/PlatformLogger/webrev.07/index.html Changes from webrev.06: - renamed back Level - LevelEnum - renamed JavaLogger - JavaLoggerProxy - moved javaLevel(int) static method from LevelEnum to JavaLoggerProxy and made it private (only used there) - consistent use of variable name 'level' to refer to PlatformLogger's int level value - consistent use of variable name 'levelEnum' to refer to LevelEnum member - consistent use of variable name 'javaLevel' to refer to java.util.logging.Level instance - consistent use of variable name 'javaLogger' to refer to java.util.logging.Logger instance - renamed PlatformLogger.newJavaLogger() private method - PlatformLogger.redirectToJavaLoggerProxy() - renamed PlatformLogger.logger private field - PlatformLogger.loggerProxy - some additional comments I think that these changes make code more consistent and self-explanatory. What remains is a possible rename from: javaLogger, javaLevel, JavaLoggerProxy - julLogger, julLevel, JulLoggerProxy if that's the final decision. Regards, Peter On 03/22/2013 01:26 PM, Peter Levart wrote: On 03/22/2013 11:34 AM, Laurent Bourgès wrote: Peter, You've beaten me! I have been preparing them too ;-) ... Ok I definitely stop working on the code and let you do it. Ok. I'll make the final touches, incorporating also comments and changes from your code... I also did some some renames, that I think make the code more consistent: - LevelEnum - Level (the code is not dependent on java.util.logging.Level, so the name can be reused, its private anyway) - julLevel - javaLevel (javaLevel / JavaLogger) - LevelEnum.forValue - Level.valueOf (Mandy) - JavaLogger.julLevelToEnum - JavaLogger.javaLevelToLevel For consistency and clarity, I would prefer having following conventions: - int levelValue (= PlatformLevel as int) and not int level (conflict with Level enum ...) I think that PlatformLogger public API should stay as is. Public method's parameter names are just called 'level' and values of public constants are expected to be passed to them. There are only two places where 'level' is the name of a local variable of type Level (and not int) and at both places there is not conflict, since there's no 'int level' variable in scope. I renamed LevelEnum to Level because the following most frequently used pattern looks strange: LevelEnum.javaLevel(int) Neither parameter nor the result has anything to do with LevelEnum directly. But if we move the javaLevel(int) method out of the Level enum into the JavaLogger, then Level can be renamed back to LevelEnum (or anything else?). - julLevel / julLogger: more explicit than javaLevel / javaLogger (java means everything ... but jul means java.util.logging and javaLogger is in conflict with JavaLogger class) But javaLogger is not in conflict with javaLevel. I suggest renaming JavaLoger class to JavaLoggerProxy, so we would have: Object javaLogger // holding java.util.logging.Logger instance Object javaLevel// holding java.util.logging.Level instance class JavaLoggerProxy // a specialization of LoggerProxy, delegating to javaLogger If 'jul' is a prefered prefix to 'java', I suggest renaming all 3: julLogger, julLevel, JulLoggerProxy. I don't have a preference for either, so perhaps Mandy, Laurent or anybody else can express them... Regards, Peter Other changes (to webrev.05): - removed the occurrence counts in switch comments (as per Mandy's suggestion) - made LoggerProxy and JavaLogger private - fixed double-read of volatile LoggerProxy.levelValue in LoggerProxy.isLoggable() - added static Level.javaLevel(int value) shortcut (Mandy) I also updated the test to exercise the correctness of mappings. Well done. cheers, Laurent
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Peter, that's OK for me and the code is clearer. What remains is a possible rename from: javaLogger, javaLevel, JavaLoggerProxy - julLogger, julLevel, JulLoggerProxy if that's the final decision. I vote for jul prefix that is more explicit; as I said, java means anything but not specifically java.util.logging. Laurent
hg: jdk8/tl/jdk: 2 new changesets
Changeset: 470232a8e89d Author:stefank Date: 2013-03-22 15:01 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/470232a8e89d 8005116: NPG: Rename -permstat option for jmap in jdk8 to -clstats Reviewed-by: jmasa, sla Contributed-by: Erik Helin erik.he...@oracle.com ! src/share/classes/sun/tools/jmap/JMap.java Changeset: 518d6087e01f Author:stefank Date: 2013-03-22 15:01 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/518d6087e01f 8004172: Update jstat counter names to reflect metaspace changes Reviewed-by: mchung Contributed-by: Erik Helin erik.he...@oracle.com ! src/share/classes/sun/tools/jstat/resources/jstat_options ! test/sun/tools/jstat/gcCapacityOutput1.awk ! test/sun/tools/jstat/gcCauseOutput1.awk + test/sun/tools/jstat/gcMetaCapacityOutput1.awk ! test/sun/tools/jstat/gcOldOutput1.awk ! test/sun/tools/jstat/gcOutput1.awk - test/sun/tools/jstat/gcPermCapacityOutput1.awk + test/sun/tools/jstat/jstatGcMetaCapacityOutput1.sh - test/sun/tools/jstat/jstatGcPermCapacityOutput1.sh ! test/sun/tools/jstat/lineCounts1.awk ! test/sun/tools/jstat/lineCounts2.awk ! test/sun/tools/jstat/lineCounts3.awk ! test/sun/tools/jstat/lineCounts4.awk ! test/sun/tools/jstat/options1.out ! test/sun/tools/jstat/options2.out ! test/sun/tools/jstat/timeStamp1.awk ! test/sun/tools/jstatd/jstatGcutilOutput1.awk
Re: RFR-8008118
Greetings: I made modifications as per Martin's suggestions: 1.) free pathv on abort. 2.) allocate memory for storing the cwd string, and then copy it into the memory location (to make sure that the contents of the pathv[] array don't refer to memory that's from the stack of the procedure that created it.) Thanks! John http://cr.openjdk.java.net/~jzavgren/8008118/webrev.06/ - Original Message - From: marti...@google.com To: chris...@zoulas.com Cc: john.zavg...@oracle.com, core-libs-dev@openjdk.java.net Sent: Friday, March 22, 2013 10:19:24 AM GMT -05:00 US/Canada Eastern Subject: Re: RFR-8008118 On Thu, Mar 21, 2013 at 12:11 PM, Christos Zoulas chris...@zoulas.com wrote: 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. Agreed. Really revert. | 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]); + Christos' suggestion looks pretty good. As Mark noted, need to add missing free of pathv itself. | 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). Agreed, pathv[i] = cwd;
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Hi Laurent, I have run your performance test on my machine (i7 CPU, Linux) and with original code on JDK8 64bit (CompressedOOPS enabled by default, no TLAB options), i get: ... INFO: testPerf[1 iterations]: duration = 1767.632977999 ms. INFO: testPerf[1 iterations]: duration = 1758.793144999 ms. INFO: testPerf[1 iterations]: duration = 1763.386362 ms. ... While with latest patch (webrev.07), the results are: ... INFO: testPerf[1 iterations]: duration = 660.940466 ms. INFO: testPerf[1 iterations]: duration = 662.485437999 ms. INFO: testPerf[1 iterations]: duration = 656.497199 ms. ... So it's apparently a 2.5x improvement in speed. This test is designed to test the PlatformLogger.isLoggable() for levels that don't result in log messages being actually written out, but there are some caveats: - the test is using a loop with local variables and counters, which is very prone to JIT optimizations such as loop-unrolling. This might or might not be happening and even if it is, the impact might be the same on unpatched vs. patched PlatformLogger code. - the test is testing the performance when the PlatformLogger is directed to java.util.logging from the beginning. That means JIT is compiling calls to virtual methods of JavaLoggerProxy into monomorphic inlined calls. The results are different if the test is 1st warmed-up while no java.util.logging is initialized yet (calls directed to LoggerProxy) and later java.util.logging is enabled (PlatformLoggers are redirected to JavaLoggerProxys) and the same test repeated. I have created a special test that demonstrates this (all tests run on recent JDK8 build, i7 (4 cores) Linux, 64bit, no TLAB option, CompressedOOPS enabled by default): ** Original code JVM START # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 1.78 ns/op (? = 0.00 ns/op) [ 1.78] 1 threads, Tavg = 1.67 ns/op (? = 0.00 ns/op) [ 1.67] # Measure: 1 threads, Tavg = 1.44 ns/op (? = 0.00 ns/op) [ 1.44] 2 threads, Tavg = 1.37 ns/op (? = 0.01 ns/op) [ 1.37, 1.38] 3 threads, Tavg = 1.53 ns/op (? = 0.13 ns/op) [ 1.41, 1.49, 1.71] 4 threads, Tavg = 1.43 ns/op (? = 0.10 ns/op) [ 1.36, 1.62, 1.39, 1.38] JVM END JVM START java.util.logging enabled # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 4.81 ns/op (? = 0.00 ns/op) [ 4.81] 1 threads, Tavg = 4.79 ns/op (? = 0.00 ns/op) [ 4.79] # Measure: 1 threads, Tavg = 4.67 ns/op (? = 0.00 ns/op) [ 4.67] 2 threads, Tavg = 4.67 ns/op (? = 0.00 ns/op) [ 4.67, 4.67] 3 threads, Tavg = 4.87 ns/op (? = 0.31 ns/op) [ 4.67, 4.68, 5.32] 4 threads, Tavg = 4.68 ns/op (? = 0.01 ns/op) [ 4.68, 4.67, 4.68, 4.69] JVM END JVM START # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 1.39 ns/op (? = 0.00 ns/op) [ 1.39] 1 threads, Tavg = 1.80 ns/op (? = 0.00 ns/op) [ 1.80] # Measure: 1 threads, Tavg = 1.38 ns/op (? = 0.00 ns/op) [ 1.38] 2 threads, Tavg = 1.38 ns/op (? = 0.00 ns/op) [ 1.38, 1.38] 3 threads, Tavg = 1.38 ns/op (? = 0.01 ns/op) [ 1.39, 1.38, 1.37] 4 threads, Tavg = 1.38 ns/op (? = 0.02 ns/op) [ 1.42, 1.37, 1.37, 1.37] java.util.logging enabled # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 11.87 ns/op (? = 0.00 ns/op) [11.87] 1 threads, Tavg = 9.08 ns/op (? = 0.00 ns/op) [ 9.08] # Measure: 1 threads, Tavg = 9.12 ns/op (? = 0.00 ns/op) [ 9.12] 2 threads, Tavg = 9.02 ns/op (? = 0.02 ns/op) [ 9.05, 9.00] 3 threads, Tavg = 9.20 ns/op (? = 0.04 ns/op) [ 9.26, 9.19, 9.17] 4 threads, Tavg = 9.33 ns/op (? = 0.07 ns/op) [ 9.44, 9.34, 9.26, 9.28] JVM END ** Patched code (webrev.07) JVM START # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 1.38 ns/op (? = 0.00 ns/op) [ 1.38] 1 threads, Tavg = 1.68 ns/op (? = 0.00 ns/op) [ 1.68] # Measure: 1 threads, Tavg = 1.38 ns/op (? = 0.00 ns/op) [ 1.38] 2 threads, Tavg = 1.40 ns/op (? = 0.02 ns/op) [ 1.37, 1.42] 3 threads, Tavg = 1.49 ns/op (? = 0.21 ns/op) [ 1.80, 1.38, 1.36] 4 threads, Tavg = 1.50 ns/op (? =
Re: 8010309 : PlatformLogger: isLoggable performance / waste due to HashMapInteger, Level leads to Integer allocations (boxing)
Peter, Laurent, This is good work. I don't have the cycle to look through this data this week. I'll get to it next week. Mandy On 3/22/2013 9:23 AM, Peter Levart wrote: Hi Laurent, I have run your performance test on my machine (i7 CPU, Linux) and with original code on JDK8 64bit (CompressedOOPS enabled by default, no TLAB options), i get: ... INFO: testPerf[1 iterations]: duration = 1767.632977999 ms. INFO: testPerf[1 iterations]: duration = 1758.793144999 ms. INFO: testPerf[1 iterations]: duration = 1763.386362 ms. ... While with latest patch (webrev.07), the results are: ... INFO: testPerf[1 iterations]: duration = 660.940466 ms. INFO: testPerf[1 iterations]: duration = 662.485437999 ms. INFO: testPerf[1 iterations]: duration = 656.497199 ms. ... So it's apparently a 2.5x improvement in speed. This test is designed to test the PlatformLogger.isLoggable() for levels that don't result in log messages being actually written out, but there are some caveats: - the test is using a loop with local variables and counters, which is very prone to JIT optimizations such as loop-unrolling. This might or might not be happening and even if it is, the impact might be the same on unpatched vs. patched PlatformLogger code. - the test is testing the performance when the PlatformLogger is directed to java.util.logging from the beginning. That means JIT is compiling calls to virtual methods of JavaLoggerProxy into monomorphic inlined calls. The results are different if the test is 1st warmed-up while no java.util.logging is initialized yet (calls directed to LoggerProxy) and later java.util.logging is enabled (PlatformLoggers are redirected to JavaLoggerProxys) and the same test repeated. I have created a special test that demonstrates this (all tests run on recent JDK8 build, i7 (4 cores) Linux, 64bit, no TLAB option, CompressedOOPS enabled by default): ** Original code JVM START # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 1.78 ns/op (? = 0.00 ns/op) [ 1.78] 1 threads, Tavg = 1.67 ns/op (? = 0.00 ns/op) [ 1.67] # Measure: 1 threads, Tavg = 1.44 ns/op (? = 0.00 ns/op) [ 1.44] 2 threads, Tavg = 1.37 ns/op (? = 0.01 ns/op) [ 1.37, 1.38] 3 threads, Tavg = 1.53 ns/op (? = 0.13 ns/op) [ 1.41, 1.49, 1.71] 4 threads, Tavg = 1.43 ns/op (? = 0.10 ns/op) [ 1.36, 1.62, 1.39, 1.38] JVM END JVM START java.util.logging enabled # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 4.81 ns/op (? = 0.00 ns/op) [ 4.81] 1 threads, Tavg = 4.79 ns/op (? = 0.00 ns/op) [ 4.79] # Measure: 1 threads, Tavg = 4.67 ns/op (? = 0.00 ns/op) [ 4.67] 2 threads, Tavg = 4.67 ns/op (? = 0.00 ns/op) [ 4.67, 4.67] 3 threads, Tavg = 4.87 ns/op (? = 0.31 ns/op) [ 4.67, 4.68, 5.32] 4 threads, Tavg = 4.68 ns/op (? = 0.01 ns/op) [ 4.68, 4.67, 4.68, 4.69] JVM END JVM START # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 1.39 ns/op (? = 0.00 ns/op) [ 1.39] 1 threads, Tavg = 1.80 ns/op (? = 0.00 ns/op) [ 1.80] # Measure: 1 threads, Tavg = 1.38 ns/op (? = 0.00 ns/op) [ 1.38] 2 threads, Tavg = 1.38 ns/op (? = 0.00 ns/op) [ 1.38, 1.38] 3 threads, Tavg = 1.38 ns/op (? = 0.01 ns/op) [ 1.39, 1.38, 1.37] 4 threads, Tavg = 1.38 ns/op (? = 0.02 ns/op) [ 1.42, 1.37, 1.37, 1.37] java.util.logging enabled # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 11.87 ns/op (? = 0.00 ns/op) [11.87] 1 threads, Tavg = 9.08 ns/op (? = 0.00 ns/op) [ 9.08] # Measure: 1 threads, Tavg = 9.12 ns/op (? = 0.00 ns/op) [ 9.12] 2 threads, Tavg = 9.02 ns/op (? = 0.02 ns/op) [ 9.05, 9.00] 3 threads, Tavg = 9.20 ns/op (? = 0.04 ns/op) [ 9.26, 9.19, 9.17] 4 threads, Tavg = 9.33 ns/op (? = 0.07 ns/op) [ 9.44, 9.34, 9.26, 9.28] JVM END ** Patched code (webrev.07) JVM START # # isLoggableFinest: run duration: 5,000 ms, #of logical CPUS: 8 # # Warm up: 1 threads, Tavg = 1.38 ns/op (? = 0.00 ns/op) [ 1.38] 1 threads, Tavg = 1.68 ns/op (? = 0.00 ns/op) [ 1.68] # Measure: 1 threads, Tavg = 1.38 ns/op (? = 0.00 ns/op) [ 1.38] 2 threads, Tavg = 1.40 ns/op (? = 0.02 ns/op) [ 1.37, 1.42] 3 threads, Tavg
hg: jdk8/tl/langtools: 7080464: langtools regression test failures when assertions are enabled
Changeset: f4500abff1fd Author:darcy Date: 2013-03-22 10:08 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/f4500abff1fd 7080464: langtools regression test failures when assertions are enabled Reviewed-by: jjg ! test/tools/javac/api/TestJavacTaskScanner.java ! test/tools/javac/diags/MessageFile.java ! test/tools/javac/diags/MessageInfo.java
Re: RFR(S): 8009397 test/com/sun/jdi/PrivateTransportTest.sh: ERROR: transport library missing onLoad entry: private_dt_socket
On 07/03/2013 11:54, Staffan Larsen wrote: Here is a webrev for fixing this problem. I actually removed all of our own tokenization code in dll_build_name() and replaced it with calls to strtok_r (strtok_s on windows) instead. I think this should be more robust, at the cost of an extra memory allocation. I've also added the const qualifier to some of the parameters. http://cr.openjdk.java.net/~sla/8009558/webrev.02/ All of the jdi and hprof tests passes with this change. Thanks, /Staffan This looks good to me too. A minor nit is that probably should put a space in while(path = ... in all of the dll_build_name implementation. Ignore this comment if you've already created the change-set are ready to push of course. -Alan
Re: RFR 8009517: Disable fatal compiler warning in the old build
On 3/22/2013 1:34 AM, Alan Bateman wrote: On 21/03/2013 22:12, Brad Wetmore wrote: : 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. No objection here but just to mention that since you need to set NEWBUILD=false then it might not be too much extra to also set JAVAC_WARNINGS_FATAL=false. At least for me, I'm not using the new build environment at all. (I think RE is doing the same when building the JCE jar files): % cd jdk/make % make381 all That is, NEWBUILD doesn't exist in this build hierarchy. As for adding JAVAC_WARNINGS_FATAL=false, I thought we were keeping warnings fatal here to so that folks running builds in this environment can the developers know they are introducing warnings/crud into their code? In any case, I think it would be desirable if there was a retirement date set for the old build so that the remaining users (I assume very few at this point) have something to aim for. Erik is now more aware of our JCE build problems, so hopefully we won't be far behind. I was surprised to hear deploy still has the dependency. Brad
Request for review fix for 8010668
This fixes a problem with the recently committed changes for JNI static libraries. On an error condition or explicit unload request, statically linked libs should not be unloaded. http://cr.openjdk.java.net/~bpittore/8010668/webrev.00/ thanks, bill
Re: RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK
On Mar 19, 2013, at 6:02 PM, Christian Thalinger
christian.thalin...@oracle.com wrote:
On Mar 19, 2013, at 5:21 PM, John Rose john.r.r...@oracle.com wrote:
On Mar 14, 2013, at 8:31 PM, Christian Thalinger
christian.thalin...@oracle.com 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:
Over all, great work on a tricky problem. I'd add a few asserts and tweak a
couple of lines; see below. Reviewed as is or with suggested changes. — John
--- Method::is_ignored_by_security_stack_walk
I would like to see reflect_invoke_cache go away some day. Would it be
possible to strengthen the asserts to prove that it is an expensive alias
for an intrinsic_id check? (I realize this is a question mainly for folks
working on JVMTI.)
That's what I tried to do: if the intrinsic_id == _invoke it also must be
the same method in reflect_invoke_cache. More than that would mean to
enhance ActiveMethodOopsCache because you can't ask for methods in the cache.
--- JVM_GetCallerClass
Suggest an assert for vfst.method() == NULL. Should not happen, and
previous code would apparently have crashed already, but...
(The corner case I'm thinking of is a compiled frame with nmethod::method
returning null after nmethod::make_unloaded. Should not happen.)
Sure, I can add that assert but there is other code in jvm.cpp that relies on
the fact that vfst.method() is non-null. We should add asserts in all that
places but that's for another RFE.
--- JVM_GetClassContext
What do these lines do:
+ // Collect method holders
+ GrowableArrayKlassHandle* klass_array = new
GrowableArrayKlassHandle();
It looks like a paste-o from another source base.
Left over. I filed an RFE for that improvement:
JDK-8010124: JVM_GetClassContext: use GrowableArray instead of KlassLink
--- LibraryCallKit::inline_native_Reflection_getCallerClass
I believe this assertion, but I would prefer to see it checked more forcibly:
+ // NOTE: Start the loop at depth 1 because the current JVM state does
+ // not include the Reflection.getCallerClass() frame.
Not sure there is a good way to do this. But, perhaps put the comment here:
case 0:
// ...comment...
ShouldNotReachHere();
How about:
case 0:
fatal(current JVM state does not include the
Reflection.getCallerClass() frame);
break;
Also, if something goes wrong with caller sensitivity, we just get a return
false. Perhaps do a caller_jvm=NULL;break to branch to the pretty
failure message? That makes it slightly easier to see what happened.
It seems easier to add printing code to the case statement:
case 1:
// Frame 0 and 1 must be caller sensitive (see JVM_GetCallerClass).
if (!m-caller_sensitive()) {
#ifndef PRODUCT
if ((PrintIntrinsics || PrintInlining || PrintOptoInlining)
Verbose) {
tty-print_cr( Bailing out: CallerSensitive annotation expected at
frame %d, n);
}
#endif
return false; // bail-out; let JVM_GetCallerClass do the work
}
break;
The LogCompilation switch should leave a paper trail. Actually, I see
that LogCompilation doesn't mention failed intrinsic inlines. Rats. At
least PrintInlining or PrintIntrinsics (diagnostic flags) will give us some
leverage if we need to debug.
--- JVM_RegisterUnsafeMethods
That's an improvement. Thanks.
(A nagging worry: How big are those static tables getting?)
We could remove some very old ones like 1.4.0 and 1.4.1. This time, next
time?
--- vframeStreamCommon::security_get_caller_frame
This now does something odd if depth 0. Suggest an assert.
The behavior with depth 0 in the current code is even worse. An assert is
a good idea. As discussed I want to remove that method in the future because
its uses are dubious.
I forgot to update the webrev. Here you go:
http://cr.openjdk.java.net/~twisti/7198429/
-- Chris
hg: jdk8/tl/jdk: 2 new changesets
Changeset: 3470101fae58 Author:weijun Date: 2013-03-23 11:49 +0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3470101fae58 8009970: Several LoginModule classes need extra permission to load AuthResources Reviewed-by: mullan ! src/share/classes/com/sun/security/auth/module/JndiLoginModule.java ! src/share/classes/com/sun/security/auth/module/KeyStoreLoginModule.java ! src/share/classes/com/sun/security/auth/module/Krb5LoginModule.java Changeset: ed63cace1d30 Author:weijun Date: 2013-03-23 11:49 +0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ed63cace1d30 8009875: Provide a default udp_preference_limit for krb5.conf Reviewed-by: valeriep ! src/share/classes/sun/security/krb5/KdcComm.java ! src/share/classes/sun/security/krb5/internal/Krb5.java ! test/sun/security/krb5/auto/KDC.java + test/sun/security/krb5/config/DefUdpLimit.java
