RFR: 8261941: Use ClassLoader for unregistered classes during -Xshare:dump

[Ioi Lam]

On Fri, 10 Sep 2021 00:20:04 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

> Before this change, unregistered classes are loaded by the boot class loader during CDS dump time.
> This RFE creates an URLClassLoader based on the source specified in the classlist and uses the URLClassLoader to load the unregistered class during CDS dump time. The URLClassLoader instances will be cached in a hash table with the source as the key so that classes associated with the same source will be loaded by the same instance of class loader.
> 
> Passed tiers 1 - 4 testing.

Changes requested by iklam (Reviewer).

src/hotspot/share/cds/classListParser.cpp line 476:

> 474:   }
> 475: 
> 476:   // This tells JVM_FindLoadedClass to not find this class.

This comment can be deleted.

The class `k` used to be defined by the boot loader. As a result, when `JVM_FindLoadedClass` looked up a class in the boot/platform/app loaders, it may inadvertently find `k`.

However, after this PR, `k` is no longer defined by the boot loader.

src/hotspot/share/classfile/classLoaderExt.cpp line 297:

> 295: }
> 296: 
> 297: class URLClassLoaderTable : public ResourceHashtable<

I think we should try to decouple the loading of unregistered classes from ClassLoader/ClassloaderExt. Maybe these new functions can be placed in a new file, shared/cds/unregisteredClasses.cpp, and the API entry point would be `UnregisteredClasses::load_class(name, path)`.

Also, `ClassLoaderExt::find_classpath_entry_from_cache` shouldn't be needed anymore. If the JAR file doesn't exist, URLClassLoader will throw an exception, so we don't need to create a ClassPathEntry just to check for the JAR file's existence.

I wonder if there's any code in classLoader.cpp that was used only for supporting the loading of unregistered classes. If so, it can be removed, too.

src/hotspot/share/classfile/classLoaderExt.cpp line 299:

> 297: class URLClassLoaderTable : public ResourceHashtable<
> 298:   Symbol*, Handle,
> 299:   7, // prime number

I think 7 is too small. Maybe 137?

src/hotspot/share/classfile/classLoaderExt.cpp line 327:

> 325: Handle ClassLoaderExt::create_and_add_url_classloader(Symbol* path, TRAPS) {
> 326:   Handle url_classloader = create_url_classloader(path, CHECK_NH);
> 327:   bool added = _url_classloader_table->put(path, url_classloader);

added is not used. Did you want to assert that `added == true`?

I think it's better to move the body of this function to line 342. Then it would be clear that the entry doesn't exist (and there's no need to check for `added`).

src/hotspot/share/classfile/classLoaderExt.cpp line 336:

> 334:     _url_classloader_table = new (ResourceObj::C_HEAP, mtClass)URLClassLoaderTable();
> 335:     Handle url_classloader = create_and_add_url_classloader(path, CHECK_NH);
> 336:     return url_classloader;

I think the above 2 lines should be removed and the creation should be done in a single place. This means `_url_classloader_table->get()` would be unnecessarily called when the table is created, but I think that's OK since this happens only once.

src/hotspot/share/classfile/classLoaderExt.cpp line 368:

> 366:   assert(result.get_type() == T_OBJECT, "just checking");
> 367:   oop obj = result.get_oop();
> 368:   InstanceKlass* k = InstanceKlass::cast(java_lang_Class::as_Klass(obj));

No need for the temp variable `k` since it's immediately returned.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5458