Laurent, I'm not subscribed to those mailing list, too. So you could
send/forward your review request to the lists yourself - no difference
here. Note that I tried sending your message to net-dev@ in the past,
and even contacted the maintainer of the mailing list via a private
email, but I never got any response and my messages never got accepted.
That's the reason I've CC'ed jdk8-dev@ yesterday...
--
best regards,
Anthony
On 4/10/2013 13:01, Laurent Bourgès wrote:
Anthony or Mandy,
Could you ask JMX / Security groups for me to review my patch ?
I am currently not registered to these mailing lists.
Do you ask me to split the patch in two part: PlatformLogger on one side
and Logger on the other side ?
Laurent
2013/4/9 Mandy Chung <mandy.ch...@oracle.com
<mailto:mandy.ch...@oracle.com>>
On 4/9/13 12:37 AM, Laurent Bourgès wrote:
Mandy,
first I would like to have the given patch applied to OpenJDK 8 (+
JDK7u) as it fixes many problems:
- string concatenations
- object creations (Object[] or other objects given as params)
- method calls in log statements
This is the patch you refer to:
http://jmmc.fr/~bourgesl/share/webrev-8010297.3/
<http://jmmc.fr/%7Ebourgesl/share/webrev-8010297.3/>
I agree that we should separate the fix to reduce the memory usage
from the fix to convert JUL to PlatformLogger. I skimmed on your
patch - awt/swing uses PlatformLogger and your change looks fine.
You have also got Anthony's approval. Other component security,
jmx, snmp, etc are using JUL. I would suggest to fix the use of
PlatformLogger in your first patch and AWT/Swing is the main area
that 8010297 is concerned about so that you can resolve 8010297 soon.
If you want to move ahead to fix use of JUL, you can send your patch
to serviceability-...@openjdk.java.net
<mailto:serviceability-...@openjdk.java.net> and
security-...@openjdk.java.net <mailto:security-...@openjdk.java.net>
for the other areas to review and sponsor.
Hope this helps.
Mandy
In a second step, I can help somebody migrating JUL usages to
PlatformLogger but it requires PlatformLogger API changes (logp to
give class and method names)...
Other comments below:
Peter's idea is a good one to add a couple of convenient
methods to take Object parameters that will avoid the creation
of array instances. I'd also like to know what variants being
used in the jdk to determine the pros and cons of the
alternatives (this issue also applies to java.util.logging).
50% of given object parameters are created via method calls so it
will not be enough.
Moreover, I prefer having always isLoggable() calls to avoid me
looking and understanding special cases (more than 2 args or no
string concat ...); besides, it would be easier to implement code
checks testing missing isLoggable() calls vs conditional and
specific checks (string concat, more then 2 args, method calls ...)
Finally, I prefer having shorter and clearer method names like
isFine(), isFinest() instead of isLoggable(PlatformLogger.FINE)
that is quite "ugly" ...
It was intentional to have PlatformLogger define only the
useful methods necessary for jdk use. The goal of
PlatformLogger was to provide a light-weight utility for jdk
to log debugging messages and eliminate the dependency to
java.util.logging and avoid the unnecessary j.u.logging
initialization (as disabled by default). It was not a goal
for PlatformLogger to be an exact mirror as
java.util.logging.Logger. My advice is to tune PlatformLogger
to provide API that helps the platform implementation while
avoid bloating the API.
Agreed.
Maybe JUL Logger.logp(classname, methodname) usages should be
rewritten to use PlatformLogger.getCallerInfo() instead:
// Returns the caller's class and method's name; best effort
// if cannot infer, return the logger's name.
private String getCallerInfo() {
String sourceClassName = null;
String sourceMethodName = null;
* JavaLangAccess access =
SharedSecrets.getJavaLangAccess();
* Throwable throwable = new Throwable();
int depth = access.getStackTraceDepth(throwable);
String logClassName = "sun.util.logging.PlatformLogger";
boolean lookingForLogger = true;
for (int ix = 0; ix < depth; ix++) {
// Calling getStackTraceElement directly prevents
the VM
// from paying the cost of building the entire
stack frame.
StackTraceElement frame =
access.getStackTraceElement(throwable, ix);
String cname = frame.getClassName();
if (lookingForLogger) {
// Skip all frames until we have found the
first logger frame.
if (cname.equals(logClassName)) {
lookingForLogger = false;
}
} else {
if (!cname.equals(logClassName)) {
// We've found the relevant frame.
sourceClassName = cname;
sourceMethodName = frame.getMethodName();
break;
}
}
}
if (sourceClassName != null) {
return sourceClassName + " " + sourceMethodName;
} else {
return name;
}
}
}
Since you are touching some jdk files that use
java.util.logging, would you like to contribute to convert
them to use PlatformLogger:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7054233
It's perfectly fine to convert only some of them if not all.
I want to help but as you said it should be done in collaboration
because it requires API changes (JMX, RMI ...) but I prefer after
this patch is reviewed and submitted and in coming weeks.
Jim Gish is the owner of logging and will check with him if he
has cycles to work with you on this.
Great.
Cheers,
Laurent
