(XS) java.logging Level.java minor cleanups
Hello, while investigating a Memory leak fix from IBM (IJ05380 is in 8.0.5.20 but not in 8u yet) I noticed that the (fixed) code registerWithClassLoader(Level) in the master queries a unused class name (pn) and it can use the method reference instead of Lambda. See patch below. There seems to be also a opportunity to use computeIfAbsent in a unrelated method add(Level), not sure if this is wanted, what do you think? index dbc27124d33..401c6dc73e0 100644 --- a/src/java.logging/share/classes/java/util/logging/Level.java +++ b/src/java.logging/share/classes/java/util/logging/Level.java @@ -610,10 +610,7 @@ public class Level implements java.io.Serializable { } private static void registerWithClassLoader(Level customLevel) { - PrivilegedAction<ClassLoader> pa = - () -> customLevel.getClass().getClassLoader(); - PrivilegedAction<String> pn = customLevel.getClass()::getName; - final String name = AccessController.doPrivileged(pn); + PrivilegedAction<ClassLoader> pa = customLevel.getClass()::getClassLoader; final ClassLoader cl = AccessController.doPrivileged(pa); CUSTOM_LEVEL_CLV.computeIfAbsent(cl, (c, v) -> new ArrayList<>()) .add(customLevel); @@ -624,19 +621,10 @@ public class Level implements java.io.Serializable { // the mirroredLevel object is always added to the list // before the custom Level instance KnownLevel o = new KnownLevel(l); - List<KnownLevel> list = nameToLevels.get(l.name); - if (list == null) { - list = new ArrayList<>(); - nameToLevels.put(l.name, list); - } - list.add(o); - - list = intToLevels.get(l.value); - if (list == null) { - list = new ArrayList<>(); - intToLevels.put(l.value, list); - } - list.add(o); + nameToLevels.computeIfAbsent(l.name, (k) -> new ArrayList<>()) + .add(o); + intToLevels.computeIfAbsent(l.value, (k) -> new ArrayList<>()) + .add(o); // keep the custom level reachable from its class loader // This will ensure that custom level values are not GC'ed I am happy if anyone can sponsor this change. BTW: I get a synthetic Accessor warning on l.name/value, is this what nestmates will resolve – is it worth adjusting the modifiers for those fields anyway? Gruss Bernd -- http://bernd.eckenfels.net
Hi Bernd, Good finding! I agree with the proposed cleanup. I am always a bit uneasy of touching these classes as they have a propensity to come back and bite you from behind when you're not looking ;-) I have applied your patch and sent it to our test system for more confidence and everything went well. I'll happily sponsor your patch. You did sign the OCA - right? MfG, -- daniel On 11/08/2018 20:31, Bernd Eckenfels wrote:
Hello,
while investigating a Memory leak fix from IBM (IJ05380 is in 8.0.5.20 but not in 8u yet) I noticed that the (fixed) code registerWithClassLoader(Level) in the master queries a unused class name (pn) and it can use the method reference instead of Lambda. See patch below.
There seems to be also a opportunity to use computeIfAbsent in a unrelated method add(Level), not sure if this is wanted, what do you think?
index dbc27124d33..401c6dc73e0 100644 --- a/src/java.logging/share/classes/java/util/logging/Level.java +++ b/src/java.logging/share/classes/java/util/logging/Level.java @@ -610,10 +610,7 @@ public class Level implements java.io.Serializable { }
private static void registerWithClassLoader(Level customLevel) { - PrivilegedAction<ClassLoader> pa = - () -> customLevel.getClass().getClassLoader(); - PrivilegedAction<String> pn = customLevel.getClass()::getName; - final String name = AccessController.doPrivileged(pn); + PrivilegedAction<ClassLoader> pa = customLevel.getClass()::getClassLoader; final ClassLoader cl = AccessController.doPrivileged(pa); CUSTOM_LEVEL_CLV.computeIfAbsent(cl, (c, v) -> new ArrayList<>()) .add(customLevel); @@ -624,19 +621,10 @@ public class Level implements java.io.Serializable { // the mirroredLevel object is always added to the list // before the custom Level instance KnownLevel o = new KnownLevel(l); - List<KnownLevel> list = nameToLevels.get(l.name); - if (list == null) { - list = new ArrayList<>(); - nameToLevels.put(l.name, list); - } - list.add(o); - - list = intToLevels.get(l.value); - if (list == null) { - list = new ArrayList<>(); - intToLevels.put(l.value, list); - } - list.add(o); + nameToLevels.computeIfAbsent(l.name, (k) -> new ArrayList<>()) + .add(o); + intToLevels.computeIfAbsent(l.value, (k) -> new ArrayList<>()) + .add(o);
// keep the custom level reachable from its class loader // This will ensure that custom level values are not GC'ed
I am happy if anyone can sponsor this change.
BTW: I get a synthetic Accessor warning on l.name/value, is this what nestmates will resolve – is it worth adjusting the modifiers for those fields anyway?
Gruss Bernd
Yes, Seeburger AG OCA should be on file (b.eckenfels@seeburger.de) Thanks for following up Did you had any opinion on the synthetic accessors as well? Gruß Bernd -- https://Bernd.eckenfels.net ________________________________ Von: Daniel Fuchs <daniel.fuchs@oracle.com> Gesendet: Donnerstag, August 23, 2018 11:20 AM An: Bernd Eckenfels; core-libs-dev@openjdk.java.net Betreff: Re: (XS) java.logging Level.java minor cleanups Hi Bernd, Good finding! I agree with the proposed cleanup. I am always a bit uneasy of touching these classes as they have a propensity to come back and bite you from behind when you're not looking ;-) I have applied your patch and sent it to our test system for more confidence and everything went well. I'll happily sponsor your patch. You did sign the OCA - right? MfG, -- daniel On 11/08/2018 20:31, Bernd Eckenfels wrote:
Hello,
while investigating a Memory leak fix from IBM (IJ05380 is in 8.0.5.20 but not in 8u yet) I noticed that the (fixed) code registerWithClassLoader(Level) in the master queries a unused class name (pn) and it can use the method reference instead of Lambda. See patch below.
There seems to be also a opportunity to use computeIfAbsent in a unrelated method add(Level), not sure if this is wanted, what do you think?
index dbc27124d33..401c6dc73e0 100644 --- a/src/java.logging/share/classes/java/util/logging/Level.java +++ b/src/java.logging/share/classes/java/util/logging/Level.java @@ -610,10 +610,7 @@ public class Level implements java.io.Serializable { }
private static void registerWithClassLoader(Level customLevel) { - PrivilegedAction<ClassLoader> pa = - () -> customLevel.getClass().getClassLoader(); - PrivilegedAction<String> pn = customLevel.getClass()::getName; - final String name = AccessController.doPrivileged(pn); + PrivilegedAction<ClassLoader> pa = customLevel.getClass()::getClassLoader; final ClassLoader cl = AccessController.doPrivileged(pa); CUSTOM_LEVEL_CLV.computeIfAbsent(cl, (c, v) -> new ArrayList<>()) .add(customLevel); @@ -624,19 +621,10 @@ public class Level implements java.io.Serializable { // the mirroredLevel object is always added to the list // before the custom Level instance KnownLevel o = new KnownLevel(l); - List<KnownLevel> list = nameToLevels.get(l.name); - if (list == null) { - list = new ArrayList<>(); - nameToLevels.put(l.name, list); - } - list.add(o); - - list = intToLevels.get(l.value); - if (list == null) { - list = new ArrayList<>(); - intToLevels.put(l.value, list); - } - list.add(o); + nameToLevels.computeIfAbsent(l.name, (k) -> new ArrayList<>()) + .add(o); + intToLevels.computeIfAbsent(l.value, (k) -> new ArrayList<>()) + .add(o);
// keep the custom level reachable from its class loader // This will ensure that custom level values are not GC'ed
I am happy if anyone can sponsor this change.
BTW: I get a synthetic Accessor warning on l.name/value, is this what nestmates will resolve – is it worth adjusting the modifiers for those fields anyway?
Gruss Bernd
On 8/23/18 2:49 PM, Bernd Eckenfels wrote:
Did you had any opinion on the synthetic accessors as well?
:
BTW: I get a synthetic Accessor warning on l.name/value, is this what nestmates will resolve – is it worth adjusting the modifiers for>> those fields anyway?
What warning message did you get? Mandy
Hello Mandy, the fields Level#name, Level#value and resourceBundleName accessed from KnownLevel inner class are private and therefore an synthetic accessor will be used. Eclipse warns about this. But, Looking at it again, it is not only a non-issue with JEP 181 in Java 11, it is also hard to fix for 8u since the Level class is public and so the fields really should stay private. (I gues I was just suprised to see any synthetic accessors in Performance critical JCL classes) Gruss Bernd -- http://bernd.eckenfels.net Von: mandy chung Gesendet: Donnerstag, 23. August 2018 22:02 An: Bernd Eckenfels; Daniel Fuchs Cc: core-libs-dev@openjdk.java.net Betreff: Re: (XS) java.logging Level.java minor cleanups On 8/23/18 2:49 PM, Bernd Eckenfels wrote:
Did you had any opinion on the synthetic accessors as well?
:
BTW: I get a synthetic Accessor warning on l.name/value, is this what nestmates will resolve – is it worth adjusting the modifiers for>> those fields anyway?
What warning message did you get? Mandy
Do you find the class files containing the bridge methods. I checked my JDK build and the Level and KnownLevel classes are in the same nest as I expect. Level is the nest host of KnownLevel which can access Level.name and other fields via getfield. I also skimmed through the class file and doesn't see the bridge method. Are you setting Eclipse with JDK 11 with nestmate support? I don't know whether Eclipse has also detected the nestmates though. Mandy On 8/23/18 3:41 PM, Bernd Eckenfels wrote:
Hello Mandy,
the fields Level#name, Level#value and resourceBundleName accessed from KnownLevel inner class are private and therefore an synthetic accessor will be used. Eclipse warns about this.
But, Looking at it again, it is not only a non-issue with JEP 181 in Java 11, it is also hard to fix for 8u since the Level class is public and so the fields really should stay private. (I gues I was just suprised to see any synthetic accessors in Performance critical JCL classes)
Gruss
Bernd
*Von: *mandy chung <mailto:mandy.chung@oracle.com> *Gesendet: *Donnerstag, 23. August 2018 22:02 *An: *Bernd Eckenfels <mailto:ecki@zusammenkunft.net>; Daniel Fuchs <mailto:daniel.fuchs@oracle.com> *Cc: *core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> *Betreff: *Re: (XS) java.logging Level.java minor cleanups
On 8/23/18 2:49 PM, Bernd Eckenfels wrote:
Did you had any opinion on the synthetic accessors as well?
:
BTW: I get a synthetic Accessor warning on l.name/value, is this
what nestmates will resolve – is it worth adjusting the modifiers for>>
those fields anyway?
What warning message did you get?
Mandy
Hi Bernd, Mandy, Sorry I missed answering your question! On 23/08/2018 21:02, mandy chung wrote:
On 8/23/18 2:49 PM, Bernd Eckenfels wrote:
Did you had any opinion on the synthetic accessors as well?
:
BTW: I get a synthetic Accessor warning on l.name/value, is this what nestmates will resolve – is it worth adjusting the modifiers for>> those fields anyway?
What warning message did you get?
I suspect these warning are emitted only by your IDE right? AFAIU then the concern therefore is mostly a perf/size issue where before nestmates [1] the compiler would have to generate synthetic accessors for accessing Level.name / Level.value from a nested inner class. It is my understanding that nestmates should remove the needs for these accessors, and given that nestmates are coming I would rather leave these fields private, rather than extending their accessibility to the whole package. [1] https://bugs.openjdk.java.net/browse/JDK-8046171 best regards, -- daniel
Mandy
Hi Bernd, It's in: https://bugs.openjdk.java.net/browse/JDK-8209987 http://hg.openjdk.java.net/jdk/jdk/rev/5f40be158613 best regards, -- daniel On 23/08/2018 20:49, Bernd Eckenfels wrote:
Yes, Seeburger AG OCA should be on file (b.eckenfels@seeburger.de)
Thanks for following up
Did you had any opinion on the synthetic accessors as well? Gruß Bernd
-- https://Bernd.eckenfels.net ------------------------------------------------------------------------ *Von:* Daniel Fuchs <daniel.fuchs@oracle.com> *Gesendet:* Donnerstag, August 23, 2018 11:20 AM *An:* Bernd Eckenfels; core-libs-dev@openjdk.java.net *Betreff:* Re: (XS) java.logging Level.java minor cleanups Hi Bernd,
Good finding! I agree with the proposed cleanup. I am always a bit uneasy of touching these classes as they have a propensity to come back and bite you from behind when you're not looking ;-)
I have applied your patch and sent it to our test system for more confidence and everything went well.
I'll happily sponsor your patch. You did sign the OCA - right?
MfG,
-- daniel
On 11/08/2018 20:31, Bernd Eckenfels wrote:
Hello,
while investigating a Memory leak fix from IBM (IJ05380 is in 8.0.5.20 but not in 8u yet) I noticed that the (fixed) code registerWithClassLoader(Level) in the master queries a unused class name (pn) and it can use the method reference instead of Lambda. See patch below.
There seems to be also a opportunity to use computeIfAbsent in a unrelated method add(Level), not sure if this is wanted, what do you think?
index dbc27124d33..401c6dc73e0 100644 --- a/src/java.logging/share/classes/java/util/logging/Level.java +++ b/src/java.logging/share/classes/java/util/logging/Level.java @@ -610,10 +610,7 @@ public class Level implements java.io.Serializable { }
private static void registerWithClassLoader(Level customLevel) { - PrivilegedAction<ClassLoader> pa = - () -> customLevel.getClass().getClassLoader(); - PrivilegedAction<String> pn = customLevel.getClass()::getName; - final String name = AccessController.doPrivileged(pn); + PrivilegedAction<ClassLoader> pa = customLevel.getClass()::getClassLoader; final ClassLoader cl = AccessController.doPrivileged(pa); CUSTOM_LEVEL_CLV.computeIfAbsent(cl, (c, v) -> new ArrayList<>()) .add(customLevel); @@ -624,19 +621,10 @@ public class Level implements java.io.Serializable { // the mirroredLevel object is always added to the list // before the custom Level instance KnownLevel o = new KnownLevel(l); - List<KnownLevel> list = nameToLevels.get(l.name); - if (list == null) { - list = new ArrayList<>(); - nameToLevels.put(l.name, list); - } - list.add(o); - - list = intToLevels.get(l.value); - if (list == null) { - list = new ArrayList<>(); - intToLevels.put(l.value, list); - } - list.add(o); + nameToLevels.computeIfAbsent(l.name, (k) -> new ArrayList<>()) + .add(o); + intToLevels.computeIfAbsent(l.value, (k) -> new ArrayList<>()) + .add(o);
// keep the custom level reachable from its class loader // This will ensure that custom level values are not GC'ed
I am happy if anyone can sponsor this change.
BTW: I get a synthetic Accessor warning on l.name/value, is this what nestmates will resolve – is it worth adjusting the modifiers for those fields anyway?
Gruss Bernd
participants (3)
-
Bernd Eckenfels
-
Daniel Fuchs
-
mandy chung