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

[Peter Levart]

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

https://dl.dropboxusercontent.com/u/101777488/jdk8-tl/PlatformLogger/webrev.06/index.html

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

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.

Have I forgotten something?


Regards, Peter

>     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 (one or the other) depends on the 'loggingSupport'
>>     flag that is initialized with PlatformLogger class. Usage of
>>     JavaLogger or LoggerProxy outside PlatformLogger would be wrong,
>>     I think.
>
>     Yes JavaLogger and LoggerProxy classes can be made private.
>
>
> Done.
>
>     I looked at it but not in great detail.  In general it's very good
>     clean up.   The comment in line 124-132 is good information and
>     since the code is evolving, I would suggest to take those count out.
>
>
> Fixed.
>
>     valueOf is one common method name to find an instance of a given
>     value.  Perhaps LevelEnum.forValue can be renamed to
>     LevelEnum.valueOf(int)?
>
>
> Done.
>
>     It seems to be useful to add a static method
>     LevelEnum.getLevel(int) to replace the calls to
>     "LevelEnum.forValue(newLevel).julLevel".
>
>
> I agree I prefer using methods (field encapsulation) so I added:
> +        /**
> +         * java.util.logging.Level optionally initialized in JavaLogger's static initializer
>
> +         * and USED ONLY IN JavaLogger (only once java.util.logging is available and enabled)
> +         */
> +        private Object julLevel;
>
> +
> +        void setJulLevel(Object julLevel) {
> +            this.julLevel = julLevel;
> +        }
> +
>
> +        Object getJulLevel() {
> +            return this.julLevel;
> +        }
> +
> +        /**
>
> +         * Return the java.util.logging.Level used only in JavaLogger
> +         * @param levelValue PlatformLogger level as int
> +         * @return java.util.logging.Level or null if java.util.logging is not available and disabled
>
> +         */
> +        static Object getJulLevel(int levelValue) {
> +            return valueOf(levelValue).getJulLevel();
> +        }
>
>
>     The tests are in jdk/test/java/util/logging and
>     jdk/test/sun/util/logging. It'd be good to check if you want to
>     extend jdk/test/sun/util/logging/PlatformLoggerTest.java to cover
>     all levels (it's currently already checking several).
>
>
> I will now have a look ...
>
> Regards,
> Laurent
>