Hi Mandy,
I have a few comments.
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.00/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java.udiff.html
private static class BootClassLoader extends BuiltinClassLoader {
BootClassLoader(URLClassPath bcp) {
- super(null, bcp);
+ super(null, null, bcp);
}
. . .
PlatformClassLoader(BootClassLoader parent) {
- super(parent, null);
+ super("platform", parent, null);
}
. . .
AppClassLoader(PlatformClassLoader parent, URLClassPath ucp) {
- super(parent, ucp);
+ super("app", parent, ucp);
this.ucp = ucp;
}
Can we give the bootstrap classloader the name "boot" or "bootstrap"?
Or this will impact too many places, and so, very risky to do?
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.00/jdk/src/java.base/share/classes/java/lang/StackTraceElement.java.frames.html
379 ClassLoader loader = cls.getClassLoader0(); The loader is unused.
402 private static String toLoaderModuleClassName(Class<?> cls) {
403 ClassLoader loader = cls.getClassLoader0();
404 Module m = cls.getModule();
405
406 // First element - class loader name
407 String s = "";
408 if (loader != null && !(loader instanceof BuiltinClassLoader) &&
409 loader.getName() != null) {
410 s = loader.getName() + "/";
411 }
412
413 // Second element - module name and version
414 if (m != null && m.isNamed()) {
415 s = s.isEmpty() ? m.getName() : s + m.getName();
416 // drop version if it's JDK module tied with java.base,
417 // i.e. non-upgradeable
418 if (!HashedModules.contains(m)) {
419 Optional<ModuleDescriptor.Version> ov = m.getDescriptor().version();
420 if (ov.isPresent()) {
421 String version = "@" + ov.get().toString();
422 s = s.isEmpty() ? version : s + version;
423 }
424 }
425 }
426
427 // fully-qualified class name
428 return s.isEmpty() ? cls.getName() : s + "/" + cls.getName();
429 }
Also, the lines 415 and 422 can be simplified: 415 s += m.getName(); 422
s += version; Also, if the loader has a name but (m == null ||
!m.isNamed()) then it looks like the sign "/" will be added twice (see
L410 and L428). It can be fixed and simplified with: Add line before
425: s += "/"; 428 return s + cls.getName();
Also, it is not clear why the loader name is not included for an instance of
theBuiltinClassLoader?
Would it make sense to add a comment explaining it?
Thanks, Serguei
On 10/25/16 16:10, Mandy Chung wrote:
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.00/
Specdiff:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/specdiff/overview-summary.html
This is a long-standing RFE for adding support for class
loader names. It's #ClassLoaderNames on JSR 376 issue
list where the proposal [1] has been implemented in jake
for some time. This patch brings this change to jdk9.
A short summary:
- New constructors are added in ClassLoader, SecureClassLoader
and URLClassLoader to specify the class loader name.
- New ClassLoader::getName and StackTraceElement::getClassLoaderName
method
- StackTraceElement::toString is updated to include the name
of the class loader and module of that frame in this format:
<loader>/<module>/<fully-qualified-name>(<src>:<line>)
The detail is in StackTraceElement::buildLoaderModuleClassName
that compress the output string for cases when the loader
has no name or the module is unnamed module. Another thing
to mention is that VM sets the Class object when filling in
a stack trace of a Throwable object. Then the library will
build a String from the Class object for serialization purpose.
Mandy
[1]
http://mail.openjdk.java.net/pipermail/jpms-spec-observers/2016-September/000550.html
