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

[Laurent Bourgès]

Hi Mandy & Peter,

Here is an update patch applying mandy's suggestions:
http://jmmc.fr/~bourgesl/share/webrev-2-8010309/

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