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 object to LevelEnum can be removed entirely.
sun.util.logging.PlatformLogger is an internal API that should only be
used by JDK. The static final int fields representing the level value
can be changed to be static final LevelEnum type instead. I checked
the JDK code that uses PlatformLogger and no code will be impacted by
the change of the type of these static fields. So it removes the need
to map from an integer value to LevelEnum. Mapping from a j.u.l.Level
to LevelEnum is trivial - the name of the LevelEnum is the same as
j.u.l.Level (e.g. LevelEnum.FINEST and Level.FINEST), you can call
LoggingSupport.getLevelName(javaLevel) to find its name and
LevelEnum.valueOf(levelName) returns the LevelEnum instance. However,
this would require more changes - basically the methods taking "int
level" as a parameter would be modified to take LevelEnum and
getLevel() would return LevelEnum too. I think it's worth doing this
cleanup to get rid of the unnecessary conversion from int -> enum ->
j.u.l.Level and vice versa. I also recommend to rename LevelEnum to
Level which is an inner class of PlatformLogger. What do you think of
this alternative to get rid of the map?
This is exactly what I was thinking too, but didn't know who uses the
API... So if the whole JDK builds with API changes, we are fine? No
outside usages?
Some other comments of your patch:
- it renames the class JavaLogger to JavaLoggerProxy and the variable
from logger to loggerProxy. I'm fine with that.
- L162: JavaLoggerProxy.init() to force load of the class which leads
to separating the initialization of LevelEnum.javaLevel in a new
JavaLevel class. The JavaLevel utility methods are not needed if we
change the static final fields to LevelEnum.
Have you tried:
Class.forName("sun.util.logging.PlatformLogger.JavaLoggerProxy",
false, PlatformLogger.getClassLoader());
would this give you the same performance improvement? If so, you can
keep the static initialization in the JavaLoggerProxy class.
Another way is: JavaLoggerProxy.class.getName(); I think it should. If
JIT only depends on loaded classes (and not whether they have already
been initialized or not). Will try...
Thanks for expanding the PlatformLoggerTest to cover additional test
cases. It's good that you compare the value of the PlatformLogger
static final fields with j.u.l.Level.intValue(). You now can use the
API to compare the LevelEnum with Level rather than reflection.
Perhaps you can add the getName() and intValue() methods in LevelEnum
class (just a thought).
Many more simplifications are possible if we change the API. Stay tuned.
Regards, Peter
Mandy
On 3/25/13 9:31 AM, Peter Levart wrote:
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:
http://dl.dropbox.com/u/101777488/jdk8-tl/PlatformLogger/webrev.08/index.html
What I did is a simple "if" deciding between two call-sites, making
sure that each is only dispatching to single class. This only works,
if both classes (LoggerProxy and JavaLoggerProxy) are loaded in
advance, before 1st invocation on any of them is made (might be that
using MethodHandles forced initialization of both classes beforehand
and hence the speed-up). If I don't load JavaLoggerProxy before
warming-up with LoggerProxy, then when j.u.logging is enabled, speed
drops for a factor of almost 4 and never catches up even after very
long time.
This pre-loading also helps for a normal single call site dispatch
that dispatches to two distinct classes, but the speed 1st drops when
the class changes, and only catches-up after several billions of
iterations (30s tight loop on i7).
Now, because JavaLoggerProxy is initialized early, I had to move the
initialization of j.u.logging.Level objects and mappings to a
separate class "JavaLevel".
Here are the benchmark results for this last iteration...
