Request Review: JDK-6479237 (cl) Add support for classloader names

[serguei.spitsyn at oracle.com]

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