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

[David Holmes]

Hi Mandy,

I know it's rather late in the game to notice this but I only just 
noticed this due to Serguei's comment ...

On 28/10/2016 5:06 PM, serguei.spitsyn at oracle.com wrote:
> 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?

Given the BootClassLoader instance is not in fact the boot loader at all 
I think it would have been clearer and avoid potential confusion to call 
this something more representative of its purpose - perhaps 
BootResourceloader or BootLoaderHelper or ...

Thanks,
David

>
>
> 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
>>
>