8010309 : PlatformLogger: isLoggable performance / waste due to HashMap<Integer, Level> leads to Integer allocations (boxing)

[Mandy Chung]

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?

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.

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).

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...
>