Dear Mandy Peter,
I will test your patch asap (+ benchmark)
Few comments below:
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8010309/webrev.00/
Please let me know if you don't agree with these changes or you find any
performance issue.
I have retained the part in
Sorry for my previous comment: JavaLoggerProxy static initialization is not
called (false argument);
I made the test:
JavaLoggerProxy: static initialization...
LoggingSupport.isAvailable: true
java.lang.Throwable
at
On 03/28/2013 10:19 AM, Laurent Bourgès wrote:
the following method in JavaLoggerProxy also caught my eye:
void doLog(Level level, String msg, Object... params) {
if (!isLoggable(level)) {
return;
}
// only
My benchmaks are also OK:
run:
-XX:+AggressiveOpts -XX:ClassMetaspaceSize=104857600
-XX:InitialHeapSize=268435456 -XX:MaxHeapSize=268435456
-XX:+PrintCommandLineFlags -XX:-PrintFlagsFinal
-XX:+UseCompressedKlassPointers -XX:+UseCompressedOops -XX:+UseParallelGC
mars 28, 2013 1:53:57 PM
Hi Peter, Laurent,
Thanks for the performance run. I'll push the changeset once I check
the test result.
On 3/28/2013 2:52 AM, Peter Levart wrote:
...just a nit! In the test:
private static void checkPlatformLoggerLevelMapping(Level level) {
// map the given level to
On 03/26/2013 10:37 PM, Mandy Chung wrote:
Hi Peter,
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.08/index.html
I'm glad that you observe similar performance improvement without the
need of method handles. I reviewed this version and realize that the
map from j.u.l.Level
Hi Mandy, Peter,
thanks for the review.
I wanted the patch to be small and only in PlatformLogger class without API
change to be easily backported to jdk7u.
Do you think this bug is JDK8 only ?
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8010309
I submitted another patch concerning
On 03/27/2013 10:32 AM, Laurent Bourgès wrote:
Hi Mandy, Peter,
thanks for the review.
I wanted the patch to be small and only in PlatformLogger class
without API change to be easily backported to jdk7u.
Do you think this bug is JDK8 only ?
I think this API level change is
Hi Mandy, Laurent,
It turns out that the API change (change of type for level parameter int
- enum Level) is entirely source-compatible. The tip of JDK8 (tl repo)
builds without a problem. So no-one is currently using
PlatformLogger.getLevel() method and assigning it to a variable or such...
Hi Peter,
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.08/index.html
I'm glad that you observe similar performance improvement without the
need of method handles. I reviewed this version and realize that the
map from j.u.l.Level object to LevelEnum can be removed entirely.
Hi Laurent, Mandy,
MethodHandles.guardWithCheck() rocks!
Out of curiosity, I put the MethodHandles in latest JDK8 build to the
test. It turns out that they are surprisingly good. I optimized the
dispatch logic of the most critical method:
PlatformLogger.isLoggable(int) which I have shown in
Hi Peter,
thanks for your microbench tool !
It works well and avoid me to do my own (caliper is a well known one)
I do not know what MethodHandle is but its performance is impressive !
Here are my results (Oracle jdk 1.7_13 vs OpenJDK 8 patched):
java -Xms8m -Xmx8m
JDK 1.7_13
run:
JVM START
Well, Laurent, Mandy,
It turns out that the dispatch speed-up (or lack of slow-down to be
precise) is possible without MethodHandles too. Maybe some VM guru could
shed some light on this, but the following is currently the fastest variant:
Peter,
Here are the results
Sorry I have no performance gain !
maybe I should use java server VM ?
Any idea ?
java -Xms8m -Xmx8m
jdk7_13
JVM START: 1.7.0_13 [Java HotSpot(TM) 64-Bit Server VM 23.7-b01]
SEVERE: PlatformLogger started
#
# isLoggableFinest: run duration: 5 000 ms, #of logical
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
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 ;-) ...
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
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
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
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
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]:
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
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
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;
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
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,
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
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()) {
//
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
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
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
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
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
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
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
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
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
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
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
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
40 matches
Mail list logo
