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

[Peter Levart]

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