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

[Peter Levart]

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